I just noticed this while looking at the details page of a watchdog report. The location field starts with double slashes (my drupal site is on the root).
Following the cause, I've obeserved that this field is populated from request_uri() which loads the data from the $_SERVER array. URIs here are prefixed with a slash, and that's what gets stored in the watchdog table.
Then, watchdog reports uses l($watchdog->location, $watchdog->location) to render the location URI, function l() calls function url() to format the URI. And it is here when base path prefixes those URLs (note that argument $absolute is FALSE).
If argument $absolute wa TRUE, the the resulting URL would look like http://www.example.com//resource (note double slashes here as well)
Conclussion: function url() generates malformed URL when $path argument is prefixed with a slash.
Not sure how to better fix, though.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | fix-47255-20060209.patch | 1.45 KB | Torenware |
| #9 | common.inc.url.rmt.patch | 623 bytes | Torenware |
| #3 | common.inc.url.patch | 426 bytes | markus_petrux |
Comments
Comment #1
chx commentedFix is possible two ways. Either we need to apply trim($string, '/'). a) fix this place only b) fix request_uri. I have looked at includes and b) causes no apparent problem there. I am wary of the two usages in modules though.
Oh, a third way is to trim on INSERT. Looking at alternatives, this looks best...
Comment #2
markus_petrux commentedThe heart of the problem is located in funtion url() though. Someone else, for whatever reason, could use url() (or function l()) with a path prefixed with a slash.
Comment #3
markus_petrux commentedhmm... in function url() if the argument $path is prefixed with a slash, it means it has already been prefixed with $base_path, right? So, all we have to do is check, if $path[0]=='/' then remove the $base_path.
Am I missing something?
Comment #4
markus_petrux commentedForgot to say I made some tests, and seems to work. But I'm afraid to say it is the best approach.
Comment #5
chx commentedI'd rather not add another check to url(). That function is not to be called with a slash'd path IMO.
Comment #6
dries commentedurl() and l() are called an aweful lot. Often, their accumulated cost exceeds the accumulated cost of all database queries.
Maybe better to modify request_uri() to return a Drupal path. In the code, Drupal paths don't have a leading '/'.
Comment #7
markus_petrux commentedBut if you tough request_uri() to not return the leading slash, that will break other places where request_uri() is used, becuase they rely on that leading slash. This is something that will probably break contrib modules.
Comment #8
markus_petrux commentedsorry: s/tough/touch
Comment #9
Torenware commentedOld patch did not work; the new patch here does work.
Comment #10
dries commentedIMO, it would be cleaner if all functions would use the same URL syntax. Having some functions return a leading slash, and some functions not, is going to be confusing.
Comment #11
Torenware commentedCases where we make no assumptions about concatenating to the $base....
form
locale
system_elements
watchdog
Cases where we make assumptions
page_get_cache (bootstrap.inc)
page_set_cache
So it appears that the fix should include a check in request_uri, and adding a slash in page_get_cache and page_set_cache.
Comment #12
Torenware commentedThe attached patch guarantees that request_uri does not return a trailing slash, and removes any code that assumes that request_uri might return a trailing slash.
Comment #13
Torenware commentedmarkus_petrux: I'm not really set up to verify that my new patch really fixes your issue with watchdog.
Could you please see if applying this patch works correctly for you? It does not seem to cause any harm, but I'm also not sure if it will really help either.
Once you can verify that this fixes the problem, Dries will give it another look.
Thanks,
Rob Thorne
Torenware Networks
Comment #14
Steven commentedPlease keep the issue title static.
Comment #15
markus_petrux commentedTorenware: I believe your patch doesn't fix the problem.
1) It is the leading slash that would have to be stripped out.
2) Note that $base_path is included request_uri, you would have to strip it too.
Not sure if this is the best approach, this way request_uri() would always be relative to $base_path, so that changes the meaning, maybe also the original purpose of that function? Can that lead to break contrib modules that need a reliable method to obtain the real request uri?
Comment #16
markus_petrux commentedAnother consideration. Watchdog uses request_uri() to capture the current location where the event occurred. Ok.
If you redirect access denied requests to your Drupal installation, then request_uri() may point to non Drupal paths. Say Drupal is not on the root folder, say you have a folder htaccess protected that generates 403 errors, that are captured by Drupal's watchdog. In this case request_uri() MUST point to the correct resource. See what I mean?
IMHO, patch in comment #3 (or something on that direction) is the way to go.
Comment #17
markus_petrux commentedWhat I mean is, IMO, request_uri() is correct as it is. What needs to be fixed is the way l() deals with absolute paths, or the way certain urls are built.
Comment #18
m3avrck commentedThis is related to another fix I have for l() as well:
http://drupal.org/node/41595
Comment #19
markus_petrux commentedI believe this has been fixed already.
Comment #20
(not verified) commented