request_uri() broken if in subdir [#10917]

Comment	File	Size	Author
#7	request_uri.broken.if.in.subdir.patch	2.6 KB	robin monks
#4	request_0.patch	2.46 KB	killes@www.drop.org
#2	request.patch	761 bytes	killes@www.drop.org
	requi.patch	678 bytes	killes@www.drop.org

Comment #1

killes@www.drop.org commented 17 September 2004 at 19:45

The patch doesn't suffice if your access only the base_url of your Drupal site in a subdir. Putting on hold.

Log in or register to post comments

Comment #2

killes@www.drop.org commented 17 September 2004 at 21:39

Status	File	Size
new	request.patch	761 bytes

ok, I think I got it. The distinction between apache and non-apache servers went away to keep it simple.

Log in or register to post comments

Comment #3

killes@www.drop.org commented 18 September 2004 at 00:19

Apparently the action in forms depends on the bug that I found, ie it needs request_uri to contain the full url (from documentroot on).

Log in or register to post comments

Comment #4

killes@www.drop.org commented 18 September 2004 at 00:53

Status	File	Size
new	request_0.patch	2.46 KB

This new patch fixes the problem with forms. request_uri() also does not start with a leading slash anymore. Ask Steven for why this is good. http://acko.net/dumpx/relative.html is apparently a clue I don't get.

Log in or register to post comments

Comment #5

Steven commented 18 September 2004 at 01:04

Explanation:

People are going to stick relative_uri() into links. These links are resolved relative to the $base_url which is put into the <head> tag. However, if a link URI starts with a / then it is relative to the root of the site, even if the base tag uses a path with a subdirectory.

Example:

<html>
<base href="http://www.test.com/subdir1/">
<body>
<a href="/subdir2">test</a>
<a href="subdir2">test</a>
</body>
</html>

The first link resolves to http://www.test.com/subdir2, the second resolves to http://www.test.com/subdir1/subdir2.

Thus, it is more meaningful to not have a leading slash, as this will avoid possible bugs with subdirs (which often do not get caught because the tester uses a site without a subdir) and hints at the callee that this is a relative URI too.

Log in or register to post comments

Comment #6

Chris Johnson commented 13 March 2005 at 18:38

Patch no longer applies completely to bootstrap.inc:

Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: includes/bootstrap.inc
|===================================================================
|RCS file: /cvs/drupal/drupal/includes/bootstrap.inc,v
|retrieving revision 1.28
|diff -u -F^f -r1.28 bootstrap.inc
|--- includes/bootstrap.inc     16 Sep 2004 07:17:54 -0000      1.28
|+++ includes/bootstrap.inc     18 Sep 2004 00:52:12 -0000
--------------------------
Patching file bootstrap.inc using Plan A...
Hunk #1 succeeded at 273 (offset 83 lines).
Hunk #2 succeeded at 292 (offset 83 lines).
Hunk #3 failed at 446.
1 out of 3 hunks failed--saving rejects to bootstrap.inc.rej
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: includes/common.inc
|===================================================================
|RCS file: /cvs/drupal/drupal/includes/common.inc,v
|retrieving revision 1.390
|diff -u -F^f -r1.390 common.inc
|--- includes/common.inc        16 Sep 2004 16:12:21 -0000      1.390
|+++ includes/common.inc        18 Sep 2004 00:52:12 -0000
--------------------------
Patching file common.inc using Plan A...
Hunk #1 succeeded at 988 (offset -90 lines).
done

Log in or register to post comments

Comment #7

robin monks commented 14 June 2005 at 11:21

Assigned:

killes@www.drop.org

» robin monks

Status	File	Size
new	request_uri.broken.if.in.subdir.patch	2.6 KB

Here is that patch re-rolled for latest CVS HEAD.

Since killes hasn't been active in this bug lately, I hope he won't mind my reassigning this to myself.

Robin

Log in or register to post comments

Comment #8

robin monks commented 14 June 2005 at 11:22

I've also tested this to work on CVS HEAD.

Robin

Log in or register to post comments

Comment #9

moshe weitzman commented 14 June 2005 at 13:33

did you test this on non apache servers? the comment that you removed is crystal clear about a need for 2 different cases. it is OK to 'make it simpler' as long as you don't remove compatibility with other web servers. I am primarily concerned with IIS.

Log in or register to post comments

Comment #10

robin monks commented 14 June 2005 at 14:42

I took great care to ensure I didn't remove any support needed for other servers. I also tested this patch to work on Xitami on Win32.

Robin

Log in or register to post comments

Comment #11

robin monks commented 14 June 2005 at 17:09

I spent some more time testing, and this broke some (perhaps all) configuration forms. It appears the / is needed after all. Perhaps killes will have better luck understanding this, the / is needed IMHO.

Robin