Ensure $_GET['q'] is set before calling drupal_normal_path()

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

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

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).

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.

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.

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

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

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?

@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.

+++ 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:

if (!request_path() && !empty($_GET['q'])) {
  print "Thinger is there."; // whatever I said in the assertion message.
}

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.

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.

Slightly confused. That test does not fail locally.

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

Hooray.

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.)

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.

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

Looks good.

Looks great! Committed/pushed to 8.x.

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

D7 reroll.

+  // 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.