Path alias memory cache is broken

Confirmed both the presence of the bug, and the fact that this fix seems to work for me. RTBC.

      if (isset($map[$path])) {
        return $map[$path];
      }
      if ($alias = db_result(db_query("SELECT dst FROM {url_alias} WHERE src = '%s'", $path))) {
        $map[$path] = $alias;
        return $alias;
      }

if there is no alias $map[$path] becomes FALSE. If you apply this patch then you have killed the caching.

Much better. Rerolled because the patch did not include the -p.

Confirmed that this too fixes the problem. Would mark back to RTBC but it already was.

+    // Delete the alias cache, since calling drupal_get_normal_path() with
+    // paths that aren't aliases may have corrupted it.

Could we make the code comment a bit more accessible? Maybe add some context. Why would paths that aren't aliases corrupt the cache? Great thing you added documentation, but I don't think the documentation is transparent enough.

I will not comment any more on this. I mailed Matt and Goba instead :)

Jaza's patch removes the same patch that went in to fix lookups: http://drupal.org/node/83234#comment-135035

I remember the patch that put those lines in. I tried getting the duplicate queries with this patch installed and didn't see any.

Committed to HEAD.

This should be backported for 4.7 too.

Still I would like to hear from the path alias masters about all this.

I would say others who submitted patches around this isse should also get contacted. Like http://drupal.org/node/65493 is a big issue which partly also deals with (and removes) this FALSE value assignment. Unfortunately I am not that deep in path aliasing now to intelligently comment on the issue.

This rolls back an important optimization. We need a different solution. If you have a Drupal site with path module turned on, with one node and one alias, this adds 16 additional database queries to the front page. Multiply that by every additional node, and you'll probably agree that this solution is not acceptable.

@drumm: to get duplicate queries on the current drupal HEAD:

1. enable path module
2. create a node
3. give it an alias
4. look at the front page.

Don't depend on the devel module to spot duplicate queries, there is a bug in it (I'm working on that at the moment).

This is the query that gets repeated:

SELECT src FROM url_alias WHERE dst = 'node'

I've simplified the whole caching scheme. This saves some processing and is easier to read. I've reinstated caching FALSE because in this function, one combination of $action and $path is going to always return the same value, every time the function is called.

As far as I can tell, the patch that got committed only forced the function to do a database lookup and conclude that FALSE should be returned instead of returning the cached FALSE.

@Jeremy, I couldn't reproduce your problem, so I probably didn't understand it correctly. To clarify, in every case, after I've edited a node, updating it's menu and path settings (thus triggering a menu cache clear), I get q=alias on the View tab and q=node/1/edit on the edit tab, and q=alias in the menu.

please have a look at my patch of http://drupal.org/node/65493
I think it could help you, too

Tobias: your patch can be rerolled slightly simpler if mine goes in. The right order for the two issues is this on first, then yours. Also, I need a half a day to understand the 5-6 lines in block_list().

Does this code need work or review?

drupal_get_normal_path does not get called as often as drupal_get_path_alias -- only with two URLs on a normal page from drupal_init_path and drupal_is_front_page. It's also called from drupal_access_denied, drupal_not_found and search_index, _menu_build and _menu_site_is_offline.

What I want to say is that the double caching has a very small memory penalty for the source part is small. I am not fond of. If mathias can find a better solution I would be happier with that .

that patch masks only the problem. if some other script would call drupal_get_normal_path with system paths then the alias cache in memory gets corruped.

I just wanted to officially retract #18. The potentially higher memory usage doesn't bother me as much as the fact that looking up a source and then a destination always results in two queries instead of the destination benefitting from the first lookup. This would be the disadvantage with any double cache, I think.

Let's begin with http://drupal.org/node/83234 for we are actually back to where we were before that went in.

What's the intention there? We want to record the negative result of SELECT src FROM {url_alias} WHERE dst = '%s'. Then let's do that and nothing else. This does not mandate two full caches.

I like Chx's approach. There was a bug in it (wrong return value for one case) and I reworked the code comments.

+      	// If there is no alias for this path, cache the path as its own alias
         $map[$path] = $path;

is false
I investigated this function very deeply in this issue
the right code is

+      	// If there is no alias for this path, cache FALSE, so we know that we already looked for it.
         $map[$path] = FALSE;

believe it or not
but drupal_lookup_path() should return FALSE if we did not find any alias or the real source.

+      if ($src = db_result(db_query("SELECT src FROM {url_alias} WHERE dst = '%s'", $path))) {
+        // Cache the $src/$path pair, but return the $src.
+        $map[$src] = $path;

$map[$src] = $path; is not good, too
Because why do you believe that $path is the same as what you would get as if you would do drupal_lookup_path('alias', 'foo');?
(this happens if you have more than one alias for any path defined)

I think there is no other way then to separate the the $map array in a part which is there for 'alias' and one for 'source'
This is what you can see in my last patch, too
If you look closely then you can see $map_source[$path] = $src;. You could argue that $map_source... is missing but that cant be defined in this case.

please try my patch it may fix your problems, too...
I know I mixed a lot of different things in this patch. (Fixing a few bugs or inconsistencies I found)

Because why do you believe that $path is the same as what you would get as if you would do drupal_lookup_path('alias', 'foo');?

How do you know what you are going to get when you run drupal_lookup_path('alias', 'foo') if there are several aliases of the same path? With the current code you have no idea which alias you will get, especially on postgresql there is no such a thing as 'table order'. So what we return after this patch is not worse than it was -- it is simply one of the many possible corrects. I updated the doxygen to reflect this situation.

It is true that $map[$path] = FALSE; is more correct i rerolled the patch as such.

I upped the wrong patch -- and suddenly it struck me that we can simplify the code further so I did.

Even less code, same principles.

Is devel module fixed?
Does the patch from #8 need to be reverted first?

I applied #31 and devel module is showing duplicate queries.

What duplicate queries? Have you tried checking by hand (for example inserting a message into the new parts of the drupal_lookup_path function)?

As this fixes some errant behaviour, too, http://drupal.org/node/78377 should get in after this.

#31 has a bug in it because isset($array) isn't working right when the array value is NULL. This version looks RTBC to me.

Scratch #35, it is just a workaround for THIS CRITICAL BUG: http://drupal.org/node/98988

So #31 is the patch to go with, and is RTBC, but we will not reap the benefits of static caching without 98988.

Any objection to applying #35 if #98988 doesn't move quickly? We can always undo the workaround later, but having working code in the meantime is nice.

I read your concerns on db_results and find them unfounded but I will coment there.

If you insist then at least please consider this one. array_key_exists is 2.5x slower than isset we learned this during form API and used the same construct.

Committed to HEAD. Sorry about the commit message, didn't notice that was already applied locally.

The extra condition in the if statement can be cleaned up once the db_result() change happens.

new regression: drupal_lookup_path returns NULL instead of FALSE

Does that last patch need to be ported to 4.7.x-dev?

I simply copypasted HEAD drupal_lookup_path, did a quick sanity review and here it is.

applied