Currently, if a module calls drupal_get_path_alias() in an implementation of hook_url_inbound_alter(), it will cause a cache miss on path aliases when viewing the front page.

drupal_lookup_path() uses current_path() as a $cid. This is fine as long as $_GET['q'] is populated, but if it isn't it tries to use an empty string as a cache id.

Normally it would be populated already, but drupal_path_initialize() calls drupal_get_normal_path() before setting $_GET['q'] in the case that it's empty (front page), so if any child function of drupal_get_normal_path() calls drupal_lookup_path(), it creates the cache miss.

I ran into this on a D7 site using the redirect module. The call chain went like this:

drupal_normal_path calls => redirect_url_inbound_alter => drupal_get_path_alias => drupal_lookup_path

The attatched patch fixes this by simply setting $_GET['q'] before calling drupal_normal_path().

Files: 
CommentFileSizeAuthor
#31 getq-d7-1329914-31.patch2.01 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,061 pass(es).
[ View ]
#28 getq-1329914-28.patch2.07 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,986 pass(es).
[ View ]
#22 getq-grr-tests.patch1.41 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,995 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#22 getq-grr-combined.patch2.01 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,983 pass(es).
[ View ]
#19 get-q-1329914-19-tests.patch1.41 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,948 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#19 get-q-1329914-19-combined.patch2 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,951 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#12 get-q-1329914-10-test-only.patch1.34 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,962 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#12 get-q-1329914-10-combined.patch1.94 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,960 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#10 get-q-1329914-10-test-only.patch1.25 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch get-q-1329914-10-test-only.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#10 get-q-1329914-10-combined.patch1.85 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch get-q-1329914-10-combined.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#7 set-get-q-before-looking-up-path-1329914-7.patch611 bytesmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 33,981 pass(es).
[ View ]
set-get-q-before-looking-up-path.patch610 bytesmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 33,773 pass(es).
[ View ]

Comments

catch’s picture

Status:Active» Needs review
catch’s picture

Issue tags:+needs backport to D7
moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-needs backport to D7

Looks good to me. This would be extremely hard to test for so RTBC.

amateescu’s picture

Issue tags:+needs backport to D7

Looks like this needs to be backported to 7.x as well, tagging.

chx’s picture

Status:Reviewed & tested by the community» Needs work

Thanks for working on this patch. It could use an automated test before it is committed. If you need help you can ask the friendly people in the #drupal-contribute IRC channel.

Also, it has a whitespace error (only one beginning space).

msonnabaum’s picture

StatusFileSize
new611 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,981 pass(es).
[ View ]

Indentation fixed. Thanks chx.

chx’s picture

the test is adding an if (!$path) { echo '$_GET[\'q\'] is set: ' . intval(isset($_GET['q'])) } to the inbound_alter in core/modules/simpletest/tests/url_alter_test.module i pointed out then adding a drupalGet('') to path.test and then asserting for $_GET['q'] is set: 1.

xjm’s picture

Assigned:Unassigned» xjm
Status:Needs work» Needs review
Issue tags:+Needs tests

I will attempt to parse chx's comment in #8 to add a test for this.

Setting NR just to send the patch to the bot.

xjm’s picture

StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch get-q-1329914-10-combined.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
new1.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch get-q-1329914-10-test-only.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Had to switch some stuff around a little, but here's test-only and combined patches.

Status:Needs review» Needs work

The last submitted patch, get-q-1329914-10-combined.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] 33,960 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new1.34 KB
FAILED: [[SimpleTest]]: [MySQL] 33,962 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Okay, totally rolled against my previous commit so those are going to fail to apply. Sorry. Here are the real ones.

kiamlaluno’s picture

Maybe I am saying something silly, but should not using a global variable that is set in url_alter_test_url_inbound_alter() and checked in the test be better?

xjm’s picture

@kiamlaluno -- I don't quite understand what you're saying; could you clarify?

I just rolled the test chx typed inline (more or less). Locally, it fails without the fix and passes with it.

xjm’s picture

+++ b/core/modules/simpletest/tests/url_alter_test.moduleundefined
@@ -30,6 +30,11 @@ function url_alter_test_foo() {
+  if (!request_path()) {
+    print '$_GET[\'q\'] is empty: ' . intval(empty($_GET['q']));
+    exit;
+  }

Though, looks like I accidentally deleted the inline comment here that explained what this hunk is for. I'll add that back after dinner.

Edit: Actually, I think I'll change it a little bit to be more self-documenting, like:

<?php
if (!request_path() && !empty($_GET['q'])) {
  print
"Thinger is there."; // whatever I said in the assertion message.
}
?>
xjm’s picture

Status:Needs review» Needs work
kiamlaluno’s picture

I was thinking of something similar to the following:

function url_alter_test_url_inbound_alter(&$path, $original_path, $path_language) {
+ global $_url_alter_q;
+  if (!request_path()) {
+   $_url_alter_q = intval(empty($_GET['q']));
+  }
+

The test then would check the value of $_url_alter_q.

xjm’s picture

Er. I'm pretty sure that won't work. :)

Assertions checking test output like that are pretty standard. See, for example UrlAlterFunctionalTest::TestCurrentUrlRequestedPath(), which has a similar function to this guy.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new2 KB
FAILED: [[SimpleTest]]: [MySQL] 33,951 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new1.41 KB
FAILED: [[SimpleTest]]: [MySQL] 33,948 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, get-q-1329914-19-combined.patch, failed testing.

xjm’s picture

Slightly confused. That test does not fail locally.

xjm’s picture

StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 33,983 pass(es).
[ View ]
new1.41 KB
FAILED: [[SimpleTest]]: [MySQL] 33,995 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Slightly modified test to avoid using exit. The test-only fails locally, and the combined passes... but then, the previous versions did too.

xjm’s picture

Status:Needs work» Needs review
xjm’s picture

Assigned:xjm» Unassigned
Issue tags:-Needs tests

Hooray.

chx’s picture

What about summarizing OP in a comment in path init: "if a module calls drupal_get_path_alias() in an implementation of hook_url_inbound_alter(), it will cause a cache miss on path aliases when viewing the front page. Setting $_GET['q'] before calling drupal_normal_path() to avoid this." (i literally copypasted OP and deleted most i didnt change a word.)

msonnabaum’s picture

Is it really necessary? Seems odd to me to have a comment about a really obscure bug when the code change is so minimal.

xjm’s picture

Well, on the other hand, I guess we might want to make sure no one reverts the fix in the future because they don't see the point. The comment could be more general.

xjm’s picture

StatusFileSize
new2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 33,986 pass(es).
[ View ]

And hey, the indentation of the original fix was a space short, so bonus.

chx’s picture

Status:Needs review» Reviewed & tested by the community

Looks good.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Looks great! Committed/pushed to 8.x.

Moving to 7.x, will need a re-roll.

xjm’s picture

Status:Patch (to be ported)» Reviewed & tested by the community
StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 37,061 pass(es).
[ View ]

D7 reroll.

webchick’s picture

Status:Reviewed & tested by the community» Fixed
+  // Ensure $_GET['q'] is set before calling drupal_normal_path().

That tells me the what, but not the why. I actually would like to see more of the why here, despite the obscurity of the bug.

So I changed that to (with chx's blessing):

+  // Ensure $_GET['q'] is set before calling drupal_normal_path(), to support
+  // path caching with hook_url_inbound_alter().

Committed #31 (in full) to 7.x, and that comment (only) to 8.x. Marking fixed.

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