Wrong sort order of aliases for different languages

Hm, that's pretty ugly.

So basically it seems like all the "ORDER BY language" stuff has to be removed from the queries in path.inc, and replaced with code in PHP that orders the languages properly?

Hrm, would using ORDER BY FIELD(language,:language,:language_none) work on only MySQL? Looks like it. :/

Actually this might work: SELECT * FROM url_alias WHERE source = :source AND language IN (:language, :language_none) ORDER BY (CASE WHEN language = :language_none THEN 1 ELSE 2 END) DESC, pid DESC

Query in #4 actually works, but it makes it hard to optimize. DamZ, andypost and I discussed adding a language_sort field that will be INT(SMALL) that will automatically store a 0 if language = LANGUAGE_NONE or 1 if otherwise. Then we just need to change ORDER BY language DESC to language_sort DESC.

Hm, @andypost, could you clarify what you mean here?

We already have tests for URL aliases, and I don't see what's wrong with adding a column in update_fix_d7_requirements().

I already said that we need to add a language_sort field here. Forcing the database to table sort is a no go.

subscribe

I tested the patch and, although it fixes the issue as advertised, it's breaking the D6 behavior, STR:

1. enable locale and add a language
2. enable multilingual support for articles
3. create an article having language english and alias 'test'
4. create an article having no language and alias 'test'
5. visit the front page

Expected behavior: both node titles link to http://example.org/test, only the english one works.

Actual behavior: just the english node title links to http://example.org/test, the language neutral node title links to http://example.org/node/2. Moreover if you change the english node's alias to 'test_en' and visit the front page again, the language neutral node title still has an unaliased link.

We need test coverage for this too.

I was wondering if a viable approach to solve this is joining on the {languages}.weight column: IMO having a priority based on the language code alphabetical order does not make much sense.

Anyway, some suggestions to (hopefully) improve comments:

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Create custom path.

"Create a custom path alias."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Check priority of language for alias by source path.

"Check the language priority for the path alias by looking up its source path."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Same check for language 'xx'.

"Perform the same check for language 'xx'."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Create language nodes to check priority of aliases.

Perhaps: "Create nodes with a language assigned to check alias priorities."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Assing custom path alias to first node with english language.

Maybe: "Assign the custom path alias to the first node and change its language to English."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Assing custom path alias to second node with LANGUAGE_NONE.

Maybe: "Assign the custom path alias to the second node and change its language to LANGUAGE_NONE."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Confirm custom path leads to first node.

Better? "Confirm that the custom path alias leads to the first node."

+++ modules/locale/locale.test	31 Jul 2010 18:07:05 -0000
@@ -1486,7 +1486,50 @@ class LocalePathFunctionalTest extends D
+    // Confirm custom path with prefix leads to second node.

Better? "Confirm that the prefixed custom path alias leads to the second node."

Powered by Dreditor.

As per the beta-3 announcement I'm asking core committers to evaluate this issue's priority: since it's partially breaking the upgrade path it might be upgraded to critical.

Ok, it seems I just have to push this to critical.

It's not clear to me why this bug must be fixed before RC1, and couldn't be fixed after RC1, or even in 7.1 or 7.2. I read the OP and plach's #21 and I still don't understand why this is critical.

As a side note, looking at the patch, that schema change needs to be moved to its own update function now that we're post-alpha.

@webchick:

My only concern is the upgrade path: we might have sites with broken/misfunctioning URLs after the upgrade, so my point is we might want to fix this before 7.0. Feel free to downgrade if this does not sound compelling.

Trying a PHP-only solution, first benchmarks seem to be encouraging.

I'll post the result of more thorough benchmarks later.

Weird failure

#26: path-863318-26.patch queued for re-testing.

Attached you can find the benchmarks on my box: a fresh installation with 5 couples of nodes on the front page each one having the two nodes sharing the same alias. I get no visible difference. I'd wish to hear a word about the patch before working on the tests (and fixing the failing ones).

I much prefer a PHP solution as we don't have to change our schemas, but I'd rather prefer a uasort() callback solution that could be re-used by other modules in contrib.

@Dave Reid:

I get your point but my solution is not actually sorting the result, it just relies on the query order, removes LANGUAGE_NONE and pushes it at the back of the result, and this should be more performant. Moreover the three queries involved in the patch will always have at maximum 2 values as result since the alias language is limited to $path_language or LANGUAGE_NONE, so there are not many possible orderings :)

Patch looks sensible to me, and preferable to a schema change. With only two results, uasort() does seem unnecessary, and the code here is perfectly readable. Didn't do a thorough review though.

This could do with tests. Since Also this could do with tests.there's no schema change it feels like not an RC blocker to me. As a regression from D6 (and potentially 404s on D6 sites after upgrade unless I misread) it should still be 'major' though.

drupal_process_path_query() is just not re-usable for contrib modules' queries, which may have more than two results. I'll work on a uasort patch tonight and compare performance, because I have a feeling that it's about the same, and will make life easier for contrib.

@Dave Reid:

If your solution has not worse performance, I don't have objections about it. Just a remark: drupal_process_path_query() is designed for resultsets of any size: it's an O(n) algorithm where an uasort in the best scenario should be O(n*log(n)).

Here's what I was working on and would be re-usable for other modules. I still need to do a performance test for before/after.

Doesn't even require two different functions. We have array_reverse for a reason...

@catch:

Since there's no schema change it feels like not an RC blocker to me. As a regression from D6 (and potentially 404s on D6 sites after upgrade unless I misread) it should still be 'major' though.

Discussed with chx about this before posting my patch, here is the chat log:

[17/11/2010 9.57.14 | Modificato 9.57.15] chx: i demoted the overlay + language issue that's no way critical
[17/11/2010 9.57.31] chx: and i am wondering how http://drupal.org/node/863318 this can possibly lead to a data loss
[17/11/2010 9.59.52] fresco cella: are you pondering if demoting that one too?
[17/11/2010 10.00.05] chx: yea
[17/11/2010 10.00.24] fresco cella: absolutely no problem from my POV
[17/11/2010 10.00.32] chx: i mean
[17/11/2010 10.00.34] fresco cella: after reading webchick's clarification
[17/11/2010 10.00.42] fresco cella: I'd say it's ok
[17/11/2010 10.00.44] chx: can it lead , possibly, to a data loss during an update?
[17/11/2010 10.01.01] fresco cella: well
[17/11/2010 10.01.15] fresco cella: it's been introduced by converting '' to 'und'
[17/11/2010 10.01.24] chx: right.
[17/11/2010 10.01.25] fresco cella: i.e. intoducing LANGUAGE_NONE
[17/11/2010 10.01.26] chx: i get that
[17/11/2010 10.01.50] fresco cella: on upgrade the data can be mapped 1:1 to the D6 one
[17/11/2010 10.01.50] chx: there was another issue imo where we pondered whether using a variable schema with one column per language owuld be better
[17/11/2010 10.02.22] chx: so the query would look like SELECT COALESCE(alias_en,alias_all) .....
[17/11/2010 10.02.28] chx: but
[17/11/2010 10.02.29] chx: too late for that
[17/11/2010 10.03.06] fresco cella: honestly I was thinking about a dead simple solution, but I wanted to ask feedback to the experts:
[17/11/2010 10.03.27] chx: what that would be
[17/11/2010 10.04.01] fresco cella: since LANGAUGE_NONE is the only 'special case', could be simply unsetting it from the result and pushing it at the end?
[17/11/2010 10.04.07] fresco cella: through PHP
[17/11/2010 10.04.46] chx: you mean, it's utterly pointless tryign to sort a result set of 2 when we know which is the more desired one?
[17/11/2010 10.04.50] chx: absolutely right
[17/11/2010 10.04.59] fresco cella: even if it was 40 results
[17/11/2010 10.05.06] fresco cella: we are looping on the result
[17/11/2010 10.05.20 | Modificato 10.05.22] fresco cella: if we just leave LANGUAGE_NONE out
[17/11/2010 10.05.28] fresco cella: and we add it at the end
[17/11/2010 10.05.29] chx: very good idea
[17/11/2010 10.05.30] chx: but
[17/11/2010 10.05.37] chx: can this lead to data loss :P ?
[17/11/2010 10.06.16] fresco cella: do you mean while we don't perform this fix?
[17/11/2010 10.07.00] chx: yes
[17/11/2010 10.07.06] chx: if the issue misses RC / 7.0
[17/11/2010 10.07.06] fresco cella: if we release D7 without it?
[17/11/2010 10.07.09] fresco cella: ok
[17/11/2010 10.07.10] chx: yes
[17/11/2010 10.07.16] fresco cella: fresco cella thinks
[17/11/2010 10.09.36] fresco cella: contrib code checking if a path alias exists and performing different actions leading to writes for the two cases, might experience errors non recoverable through the bug fix
[17/11/2010 10.09.56] chx: hrm, ok, let's leave at riitcqal
[17/11/2010 10.10.06] chx: please file this simpler userspace solution
[17/11/2010 10.10.18] fresco cella: will do :)

@plach, I followed the steps in #21 using HEAD (no patch), and

Expected behavior: both node titles link to http://example.org/test, only the english one works.

This was not the case. While both titles linked to /test, the language-neutral node was displayed. Do we have a regression in HEAD then?

By the way, the language I added was Lolspeak, in case that matters :)

#38: 863318-path-language-sorting-D7.patch queued for re-testing.

lolspeak is xx-lolspeak so it's after und. There are not many languages after und.

I see, so we need a test to expose the ordering problem in HEAD, when adding a language with a code starting with one of 'vwxyz'.

I'm working from #38, trying to salvage tests from #20. Additionally, the unaliased link bug in #21 should get a test.

I don't think sorting the whole result is necessary. A foreach() which does break; as soon as it finds a non-LANGUAGE_NONE alias should be sufficient. I reversed the direction of the ORDER BY clause to make this work. I don't think it's critical to make this super contrib-friendly; the procedure of an uasort() and conditional array_reverse() is not exactly a friendly "API" either :)

I included the tests from #20, and added an additional bit to test for the bug in #21 (both node titles should link to the same alias).

This looks good but i do not understand why $aliases = db_query("SELECT alias, language FROM {url_alias} WHERE source = :source AND language IN (:language, :language_none) ORDER BY language ASC, pid ASC" differs in ordering. Why is that not DESC? if a path has several aliases then this query will have different results than D6. Let's see what the bot has to say about this.

Ahh, I wrote #47 and didn't interpret the behavior of the fetch methods correctly. There is an important difference here between fetchAllKeyed() and fetchField() in terms of what happens when multiple rows match the key. fetchAllKeyed() will return you the bottom result while fetchField() returns the top. With #47 and #48 we are replacing that with a simple loop that prefers the top, so we must adjust the order clause appropriately (X means a mistake):

HEAD:

alias cache: fetchAllKeyed(), asc, prefer bottom
alias not cached: fetchField() desc, prefer top
source: fetchField() desc, prefer top

#47:

fetchAll() desc, prefer top
fetchAll() asc, prefer top (X)
fetchAll() asc, prefer top (X)

#48:

fetchAll() desc, prefer top
fetchAll() desc, prefer top
fetchAll() asc, prefer top (X)

#49:

fetchAll() desc, prefer top
fetchAll() desc, prefer top
fetchAll() desc, prefer top

In #49 we consistently use DESC and prefer the top, which should match the behavior of HEAD while leaving LANGUAGE_NONE as a fallback.

As the above bot passes show, the tests we have so far aren't sufficient enough to catch precedence errors. So we'll need more tests in that area.

I've also included a database test I wrote which helped me learn/verify the behavior of these fetch methods. If no one objects, I think it might be a good addition here.

Let's remove the db test. @chx explained that there is no precedence intended because normally fetchAllKeyed() and fetchField() are supposed to work on one result per key anyway. In path.inc we have an oddball situation and it might be a misuse of fetchAllKeyed() and fetchField() since there are multiple rows matched.

What we really need is a test to make sure the expected alias is returned when there are multiple aliases for a source/path, and vice-versa. The patches in #47 and #48 mixed that up without any test failures.

In other news, why would "node body (broken) - 37" found fail here? upgrade.node.test line 32

@andypost can you explain what difference it makes to use path_save() versus drupalPost() in the test?

#52: 863318-path-lang-sort-D7.patch queued for re-testing.

The test fail is because drupal-6.filled.database.php has *two* LANGUAGE_NONE aliases of 'content/1263769200', which point to different nids, node/13 and node/37. That is silly and probably just resulted from manually editing a db dump. The bug is exposed by the patch here sorting DESC and using the second result if both have LANGUAGE_NONE, so it uses node/13. I would call that a bug in the test db and not the patch.

However, there is a real problem with #47-and-on that the path cache is no longer used completely in the first hunk. I'll try to remedy this, and look into writing tests for multiple-lang-aliases-for-one-source.

This fixes the first hunk which previously bungled the path cache. The first query is now back to ORDER BY language ASC, pid ASC to help fix that, since we can't use the simple break; method like the 2nd and 3rd queries.

Didn't have time to write additional tests yet.

Now after looking at path_save() I'm starting to doubt our method here. Although the path module forbids a language to have multiple aliases for a path via validation, there is no such validation when using path_save() and a language (especially LANGUAGE_NONE) might have many aliases per path, the effective one being MAX(pid). If the given language is LANGUAGE_NONE, my patch would effectively return MIN(pid), which is not so good. Needs more work then.

Went back to @Dave Reid's approach, after all that. Cleaned it up so it should now pass tests, added a docblock for drupal_sort_url_aliases(), and tested it, looks like we consistently now return MAX(pid) when language is found, then fallback to MAX(pid) LANGUAGE_NONE. That should make sense if there are multiple aliases in the table for a path, created directly with path_save() without specifying a language.

Am I wrong or we still need benchmarks for this approach?

#58, carlos, please get back to the previous, simpler method. ordering on pid DESC and bailing out on the first non-und entry results in the MAX(pid).

@chx, if we go back to the break; method, we need to fix this:

$alias1 = array(
  'source' => 'node/1',
  'alias' => 'test',
);
path_save($alias1);

$alias2 = array(
  'source' => 'node/1',
  'alias' => 'test2',
);
path_save($alias2);

print drupal_lookup_path('alias', 'node/1');

Although we return MAX(pid) if the language is specified, the approach in #57 returns MIN(pid) otherwise and the above code returns 'test' instead of 'test2'. If we can fix this in a way that doesn't make the code unreadable, and still performs better than #59, I am all for it.

Also if we want to reduce memory usage, we might want to avoid doing fetchAll(). The above example shows that {url_alias} can contain many duplicate aliases for any given language. If memory is really an issue, a schema change with a language_none bit would allow us to keep the existing fetchAllKeyed() and fetchField() calls and not load duplicate aliases into PHP, but then we have to deal with community pushback from a schema change this late.

It would also be nice to add the above example to path.test so we can prevent against this type of regression. The upgrade.node.test failure are the only thing that caught this regression so far.

Correction, fetchAllKeyed() would still load duplicate aliases into memory (for the path cache), but fetchField() wouldn't (for individual lookups).

Also, it seems odd that path_save() doesn't do a db_merge on (source, language) if pid isn't provided. As it is there could be duplicate aliases floating around that are never used.

This avoids fetchAll and is based on #48 and passes the tests in #59 ie. I still dislike uasort here.

Poor patch got ignored due to poor naming? Let's try this.

=== modified file 'includes/path.inc'
--- includes/path.inc	2010-08-09 00:13:06 +0000
+++ includes/path.inc	2010-11-23 04:39:18 +0000
@@ -94,13 +94,21 @@ function drupal_lookup_path($action, $pa

Maybe a problem in how you rolled the patch?

There are a few things I don't get about this patch:

• Why use ORDER BY language DESC? I think that is a legacy thing because LANGUAGE_NONE used to be just a blank string, which would sort at the top of the list, thus using DESC and limiting the result is a way to return a language-specific alias if there is one. Now that LANGUAGE_NONE is 'und', the language DESC sort is no longer relevant. MAX(pid) should definitely be honored, though.
• Although we are not using fetchAll(), the patch pretty much duplicates what fetchAll() does anyway, so there is no memory usage improvement. What I meant by not using fetchAll() was that if we could iterate and break; without processing the whole result, then less memory is used. But in #66 it's the same as fetchAllKeyed() with language being the key, I think.
• #59 looks cleaner. usort() can't be that bad for performance, and at least makes more readable and reusable code. I don't think there's a reason to dislike usort() unless the benchmarks show that it lags behind. As far as benchmarking, and how many duplicate aliases we should expect to have in the {url_alias} table for LANGUAGE_NONE I am not sure, I would defer to @Dave Reid on that.

Hm, I thought that using language ordering was necessary but you might be right that it's not, actually. It needs update indexes if it's not. The query might be faster, even. No, the patch does not replicate fetchAll because it does not store every result. I am happy to ask Dave Reid which approach he likes better. I consider my code pretty readable and straightforward of course :)

I'm going to prefer the uasort() solution since I know I can use in at least one module that I co-maintain. And I would be under the assumption that it would be ok to use the admittedly slower uasort() solution since the results of all these queries/sorts are cached as well via drupal_cache_system_paths() so that the next same page load wouldn't hit the db?

Well let's get some benchmark comparisons of HEAD, #59 (usort), and #66 (foreach) then, and if usort() performs OK, go with that. Preferably we test on a real D6-upgraded site with lots of aliases from pathauto. Edit: and multiple languages, if we can.

drupal_cache_system_paths() in no way caches the alias fetching, that's why it's called drupal_cache_system_paths() ;). All it does is turn lots of little queries into one big IN() query, the PHP cost of sorting out the individual aliases will be the same.

Let's add more comments to the code. I'd like to see us outline the intended behavior somewhere in the code comments to people can follow along. Also, the 'und' problem is not explained in the code. It was not discoverable from reading the proposed patch.

+++ includes/path.inc	2010-11-23 04:39:18 +0000
@@ -94,13 +94,21 @@ function drupal_lookup_path($action, $pa
+          // Ordering on pid ASC means the last record per language will be the ¶
+          // highest pid. Keep only that one.

I recommend that we explain _why_ we want to keep the last one.

+++ includes/path.inc	2010-11-23 04:39:18 +0000
@@ -94,13 +94,21 @@ function drupal_lookup_path($action, $pa
+          // An alias in $path_language is preferred.

Would be good to extend this code comment. Why would it be preferred?

Here are some benchmarks. I used catch's "micro benchmark" technique of calling an API function in a loop, to emphasize that function in the results. The test file:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

for ($i = 0; $i < 100; $i++) {
  print drupal_lookup_path('alias', 'node/3') . '<br />';
  drupal_static_reset('drupal_lookup_path');
  print drupal_lookup_path('source', 'test') . '<br />';
  drupal_static_reset('drupal_lookup_path');
}

The test site is a standard D7 install, 8 nodes in the database, with several aliases for node/3 and several sources for 'test' in different langauges (English is the default on the site).

HEAD

Document Path:          /test.php
Document Length:        2200 bytes

Concurrency Level:      1
Time taken for tests:   129.158 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      2568000 bytes
HTML transferred:       2200000 bytes
Requests per second:    7.74 [#/sec] (mean)
Time per request:       129.158 [ms] (mean)
Time per request:       129.158 [ms] (mean, across all concurrent requests)
Transfer rate:          19.42 [Kbytes/sec] received

#59

Document Path:          /test.php
Document Length:        2200 bytes

Concurrency Level:      1
Time taken for tests:   136.679 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      2568000 bytes
HTML transferred:       2200000 bytes
Requests per second:    7.32 [#/sec] (mean)
Time per request:       136.679 [ms] (mean)
Time per request:       136.679 [ms] (mean, across all concurrent requests)
Transfer rate:          18.35 [Kbytes/sec] received

#65

Document Path:          /test.php
Document Length:        2200 bytes

Concurrency Level:      1
Time taken for tests:   127.228 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      2568000 bytes
HTML transferred:       2200000 bytes
Requests per second:    7.86 [#/sec] (mean)
Time per request:       127.228 [ms] (mean)
Time per request:       127.228 [ms] (mean, across all concurrent requests)
Transfer rate:          19.71 [Kbytes/sec] received

#65 and HEAD are neck-and-neck, and #59 was slightly behind. So as expected, #59 comes with a small performance regression but makes the sorting mechanism usable in contrib while also being a little more self-documenting.

Tough choice! Dries had trouble reading the logic of #65... which would have to be documented in multiple places. #59 however is more encapsulated and can be documented centrally.

I also did some more general benchmarks using <front> and /xx-lolspeak/test, and I couldn't get a definitive reading that was out of the margin of error. Therefore, within the context of a normal page load, the performance regression of #59 is probably extremely minor.

I suggest we dust off #59, document it a little bit better, and then we can be done here :)

I think I'd be OK with #65 if it were better documented. #59 seems like it introduces a 5% performance regression which seems too much.

@Dries: to be clear, this is not a 5% regression of an actual page load, because this is a "micro" benchmark: I used test.php to loop 100x and call drupal_lookup_path('alias', ...) and drupal_lookup_path('source', ...), to make the gain/loss magnified. So it's a ~5% regression in an artificially magnified test, which shows that it is indeed slower, but not by much. The difference shouldn't be noticeable in actual practice.

@carlos8f:

Why not introducing a new API function as in #26 to encapsulate the fetching logic? We would end with performant yet reusable code.

@carlos8f: I know it is micro-code. drupal_lookup_path() is called a lot though -- sometimes hundreds of times per page. Imagine a node with a hundred comments -- that quickly translates to 200+ links, and therefore 200+ calls to drupal_lookup_path(). If you're an administrator on that site, it quickly becomes 400+ links on such a page.

Trying to work out something based on @plach's suggestion.

Here is a more encapsulated approach (very usable in contrib), with the fallback logic simply implemented as an operator: $aliases[$path_language] + $aliases[LANGUAGE_NONE].

I also added some unit test goodies in path.test which ensure that the correct thing is returned, given a variety of situations. The update function # needed to be bumped, as well.

Looks like another round of micro benchmarks is in order.

It's not re-usable for contrib when we need all results still, which is why I still preferred the uasort().

@Dave Reid, can you elaborate on that? Do you mean that you'd rather keep the query separate from the sorting process, as in #26?

Is there a method that is usable in contrib but doesn't cause a performance regression? @Dries has stated that the ~%5 performance loss caused by the usort() method would be unacceptable.

#79: 863318-79.patch queued for re-testing.

If I'm understanding right, contrib needs a way to purely sort aliases without collapsing $path_language and LANGUAGE_NONE?

Since core doesn't provide that currently, I don't see why that's a requirement of this patch going in, which just fixes existing but broken behavior.

Fixed that bug by applying an array_reverse() to the return value, and added a test for it:

+    // Test the situation where the alias and language are the same, but
+    // the source differs. The newer alias record should be returned.

#86: 863318-86.patch queued for re-testing.

#86: 863318-86.patch queued for re-testing.

New benchmarks in, using more magnification in the micro loop:

ab -c 1 -n 100

And now test.php loops 1000x instead of 100x.

HEAD

Document Path:          /test.php
Document Length:        12000 bytes

Concurrency Level:      1
Time taken for tests:   78.510 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1234700 bytes
HTML transferred:       1200000 bytes
Requests per second:    1.27 [#/sec] (mean)
Time per request:       785.101 [ms] (mean)
Time per request:       785.101 [ms] (mean, across all concurrent requests)
Transfer rate:          15.36 [Kbytes/sec] received


#59

Document Path:          /test.php
Document Length:        12000 bytes

Concurrency Level:      1
Time taken for tests:   79.622 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1234700 bytes
HTML transferred:       1200000 bytes
Requests per second:    1.26 [#/sec] (mean)
Time per request:       796.219 [ms] (mean)
Time per request:       796.219 [ms] (mean, across all concurrent requests)
Transfer rate:          15.14 [Kbytes/sec] received

#65

Document Path:          /test.php
Document Length:        12000 bytes

Concurrency Level:      1
Time taken for tests:   80.577 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1234700 bytes
HTML transferred:       1200000 bytes
Requests per second:    1.24 [#/sec] (mean)
Time per request:       805.773 [ms] (mean)
Time per request:       805.773 [ms] (mean, across all concurrent requests)
Transfer rate:          14.96 [Kbytes/sec] received

#86

Document Path:          /test.php
Document Length:        12000 bytes

Concurrency Level:      1
Time taken for tests:   133.370 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1234700 bytes
HTML transferred:       1200000 bytes
Requests per second:    0.75 [#/sec] (mean)
Time per request:       1333.705 [ms] (mean)
Time per request:       1333.705 [ms] (mean, across all concurrent requests)
Transfer rate:          9.04 [Kbytes/sec] received

With more magnification, we are now pushing past the margin of error. It looks like the usort() method (#59) is very close to HEAD. I had a hard time believing this so I ran the results several times, and it seems to be true. So I think we should still consider the usort() method.

Throwing this idea here. Will add comments.

Passes tests. I'm not sure how, but it works :) Benchmark looks good...

Document Path:          /test.php
Document Length:        12000 bytes

Concurrency Level:      1
Time taken for tests:   78.304 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      1234700 bytes
HTML transferred:       1200000 bytes
Requests per second:    1.28 [#/sec] (mean)
Time per request:       783.037 [ms] (mean)
Time per request:       783.037 [ms] (mean, across all concurrent requests)
Transfer rate:          15.40 [Kbytes/sec] received

This patch's method barely makes sense though. The usort() (#59) looks like the most readable/performing patch so far. Update: I re-ran the benchmark on #59 and got 786.980 ms average, which is 1 ms more than HEAD.

Also, it's been mentioned in #2 but can we get an opinion on this: is it really necessary that we store language-neutral aliases as 'und' in the database? If we rolled that back to a blank string, that would seem to solve the whole issue here.

barely makes sense? I am reusing the query setting the direction as dictated. I would be VERY cautious about re-encoding und as '' in the database (especially this late).

With comments.

I'm not sure why #96 doesn't make sense, if we only have one language + und to check at any one time, then changing the sort order like that works for me. As long as we can order in SQL without a schema change or unindexed query then there's no reason to do this in PHP.

It must've been late last night; the patch does make sense especially with the comments. I cleaned up a couple grammar mistakes and added a note about pid ASC/DESC. Also, $path_language == LANGUAGE_NONE on the vast majority of sites, so I thought it was worth adding a case for.

Glad to hear there is consensus. Let's see what the test bot has to say. We're close, folks!

While the grammar changes are OK the query changes are not and I removed them. You can't remove a column from the middle of an index and expect it to work. Those == queries would've been unindexed.

  PRIMARY KEY (`pid`),
  KEY `alias_language_pid` (`alias`,`language`,`pid`),
  KEY `source_language_pid` (`source`,`language`,`pid`)

@chx I think you might be mistaken about the query being unindexed:

EXPLAIN SELECT alias FROM url_alias WHERE source = 'node/3' AND language IN ('und', 'und') ORDER BY language DESC, pid DESC

EXPLAIN SELECT alias FROM url_alias WHERE source = 'node/3' AND language = 'und' ORDER BY pid DESC

Well, then #100 is necessary becauseEdit: it failed because PDO wont tolerate passing in extra arguments. It passed while #98 didnt. Yes we could manipulate the $args array into submission but this is rapidly getting too much code for way too little benefit.

Edit: MySQL is braindead to treat IN (foo, foo) as a range query. Go for the optimization then.

Here we avoid a useless "range" query for IN ('und', 'und'), which is braindead as @chx says.

chx's imaginary webchick filter asked me to add a code comment for unset($args[':language]): "Prevent PDO from complaining about a token the query doesn't use."

I think we are good to go.

I think this looks great now. Committed to CVS HEAD. Thanks all.

Hello,
I set Multilingual settings taxonomy vocabulary on Localize terms. then for taxonomy term pages, I created different aliases for each language.
on my menu, i'm always getting the alias for the undefined language, even when i'm on a different language than the default one.
I think this is because of the queries generating the cached alias data. the "order by" statment seems not ok.
Actually the queries (at line 111) are different from the ones fiew lines later.
this patch solved my problem.

chikipi is right. We have different behaviors for the cases when $cache['first_call'] is empty and when it's not. For example, let's assume that we have current language "de", and we have the following records in the url_alias table:

pid  source           alias  language
===  ===============  =====  ========
1    taxonomy/term/1  seite  de
2    taxonomy/term/1  page   und

Case 1, $cache['first_call'] is empty.
Will be executed query from line 114. Language order is ASC. And fetchAllKeyed() will return array('taxonomy/term/1' => 'page', ...).

Case 2, $cache['first_call'] is not empty.
Will be executed query from line 150. Language order is also ASC. But fetchField(), which is used in this case, will return "seite".

I guess the behavior in the first case is wrong, and the condition on line 113 should be reversed.

The same is reported here: https://drupal.org/node/2065977

Attached patches demonstrate the issue.

(Assuming this is ready for review).

Does #21 still have the best means to test this?

Do we need more tests than in #109? Patch still applies nicely.

@carlos8f last contributed to this issue 4 years ago.

Looks like this was elevated in #23 as a way to get core committer attention. I can't see anything in this issue that would qualify as critical, however. It does not:

* Render [the] system unusable and have no workaround.
* Cause loss of data.
* Expose security vulnerabilities.

It seems to be solidly major, however.

The patch #109 is still relevant. That sort condition has to be reversed because fetchAllKeyed() always returns the last record if more than one record with the same key were found. The supplied test also works well for me.

Function drupal_lookup_path() uses ORDER BY language DESC, pid DESC to search for alias or source of alias.

This caused by LANGUAGE_NONE is now 'und' so idea "...The language should have priority over the empty language." is totally broken because there are different languages

So aliases with LANGUAGE_NONE have priority on languages that starts from 'u' letter - most of languages

Suppose this is a major issue that affects upgrade path