It's seems that calling drupal_get_path_alias() from within hook_url_inbound_alter() effectively causes the cache path to break.

Perhaps resetting $cache['first_call'] to TRUE in redirect_url_inbound_alter() after calling drupal_get_path_alias() will fix the problem?

If you enable devel query log, you'll see that the cache_path is being updated on each request.

Comments

gielfeldt’s picture

Note: $cache refers to &drupal_static('drupal_lookup_path');

mfb’s picture

Status: Active » Needs review
StatusFileSize
new789 bytes

I am seeing this too. I believe it happens on any page which is a path alias.

Since this appears to be dead code, the easiest fix would be to comment it out.

berdir’s picture

Priority: Normal » Critical
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Performance

Yes, this is a critical performance issue. Found this out myself too.

pere orga’s picture

Wouldn't be better to delete the code instead?

pere orga’s picture

Priority: Critical » Major
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new1 KB

there are no references to 'redirect_canonical' variable. Let's remove it. Changing the status to 'major' but feel free to change it if you feel strong about it.

mfb’s picture

Status: Needs review » Reviewed & tested by the community

Deleting nonworking code sounds great to me :)

My only comment would be that if this variable used to exist in a previous version of the module, then an update script should delete it.

dave reid’s picture

Status: Reviewed & tested by the community » Needs work

Let's comment out the code please instead of deleting.

gielfeldt’s picture

In my experience, committing commented out/disabled code is usually a bad idea for several reasons. VCS is not a backup archive, and can it can be potentially problematic determining the correct history of the code.

dave reid’s picture

There is intent to make this code work. I, the maintainer, would rather it stay commented-out so I can move it and un-comment it, rather than me having to go back and find the hash where the code existed.

pere orga’s picture

In that case, in patch #2 I would comment out this line too:

if ($path && variable_get('redirect_canonical', 1)) {
mfb’s picture

Status: Needs work » Needs review

patch at #2 still applies

fabianx’s picture

Status: Needs review » Needs work

CNW per #7, this won't be applied unless a proper patch is made per maintainer policy.

mfb’s picture

Status: Needs work » Needs review

Can you review the patch at #2?

fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Oh, I see what you mean. Woops.

RTBC for #2. I was confused.

azinck’s picture

Got bitten by this, too. Can confirm #2 works well.

heddn’s picture

StatusFileSize
new1.39 KB

Leaving this as RTBCed, since #18 just comments out the wrapping conditional and adds a comment back to this issue.

azinck’s picture

While this is certainly code that should be removed/commented-out, the root of this problem is really an issue with core: #2400539: Path cache won't always contain the canonical alias, depending on order of calls to drupal_lookup_path()

  • Dave Reid committed 6e618dd on
    Issue #2048137 by mfb, Pere Orga, heddn: Disable canonical redirect code...
dave reid’s picture

Status: Reviewed & tested by the community » Fixed

Ok very interesting that even those these API functions are available at this point, they cause data integrity. Strange. I'll keep this in mind for when I come back to looking at this code, so committing #18 for now.

Status: Fixed » Closed (fixed)

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