It is fairly common to move rewrite rules into the Apache vhosts file. One reason for this might be to perform rewriting early and then use LocationMatch rules to perform various actions. The problem with doing so is that $_GET['q'] will likely get an extra '/' prepended to it. Meaning that for example what used to be 'node/42' is now '/node/42'. This usually causes no problems because in drupal_path_initialize() we first

$_GET['q'] = drupal_get_normal_path(trim($_GET['q'], '/'));

before we do anything with it. the trim() removes any leading slashes. However DRUPAL_BOOTSTRAP_LANGUAGE happens before DRUPAL_BOOTSTRAP_PATH. Here we currently do no such cleanup:

$args = isset($_GET['q']) ? explode('/', $_GET['q']) : array();
$prefix = array_shift($args);

If there is a leading slash this causes big problems because the first element of the array is now empty instead of a language code.

I did some research to see if I could find any notes on why the trim() was added to drupal_path_initialize() in the first place. This code was committed in 2003 with a very vague commit message:

http://drupal.org/cvs?commit=608

So that doesn't help much.

I'll have a patch shortly.

CommentFileSizeAuthor
#12 language-587706-12.patch1.94 KBplach
#8 language-587706-8.patch1.94 KBplach
#3 language-587706-3.patch714 bytesplach
#1 language_trim_slashes.diff825 bytesdalin

Comments

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
StatusFileSize
new language_trim_slashes.diff825 bytes

simple patch using ltrim()

aaron’s picture

aaron commented

subscribe

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Active » Needs review
StatusFileSize
new language-587706-3.patch714 bytes

Rerolled

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Title: language_initialize() needs to clean up $_GET['q'] just like drupla_path_initialize() does » language_initialize() needs to clean up $_GET['q'] just like drupal_path_initialize() does
Status: Needs review » Reviewed & tested by the community

Green. I think it's safe to set this RTBC.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Issue tags: +Quick fix

tagging

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Good catch! Committed to HEAD.

Although, upon reflection, is there any other more "central" place we could move that trim() to so we didn't have to do it in both places?

Temporarily marking "needs work" to answer that question; feel free to bump to "fixed" if the answer is no.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

I thought about this for a while before rerolling the patch: IMO the answer is no because drupal_path_initialize() is always executed, while language_initialize() is executed only in multilingual sites; OTOH language initialization happens before path initialization so they can't rely on one another for polishing $_GET['q']: we should have a dedicated phase happening before.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
StatusFileSize
new language-587706-8.patch1.94 KB

I gave this a deeper look: the right place for centralizing $_GET['q'] normalization might be http://api.drupal.org/api/function/request_path/7.

Let's see if the bot complains.

Status: Needs review » Needs work
Issue tags: -Quick fix

The last submitted patch, language-587706-8.patch, failed testing.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs work » Needs review
Issue tags: +Quick fix

#8: language-587706-8.patch queued for re-testing.

Cannot reproduce any error on my box.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Needs review » Reviewed & tested by the community

Looks great to me.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
StatusFileSize
new language-587706-12.patch1.94 KB 
+++ includes/bootstrap.inc	9 Mar 2010 23:55:01 -0000
@@ -2188,6 +2188,11 @@ function request_path() {
+  // assigned to $_GET['q] with a slash. Moreover we can always have a trailing

Fixed the above typo: $_GET['q].

Powered by Dreditor.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Great. Much cleaner. :)

Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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

Issue tags: +Quick fix

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.