Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicholasThompson’s picture

Assigned: WiseTeck » nicholasThompson
Priority: Normal » Critical
Status: Needs review » Needs work

This is a good point...

My concern here is if someone (stupidly) calls their nested bootstrap index.php, then this patch will cause issues too.

So, lets take a look at the opions... Drupal could be installed in:

  • /index.php
  • /subfolder/index.php

Based on these two scenarios, the modules could then have a bootstrap in...

  • /sites/all/modules/mymodule/ad.php
  • /subfolder/sites/all/modules/mymodule/ad.php

Your patch solves these issues, but what if a developer releases a module with a bootstrap as...

  • /sites/all/modules/mymodule/index.php
  • /subfolder/sites/all/modules/mymodule/index.php

So essentially - for a given Drupal Install, what's the difference between these two paths?

  • /subfolder/index.php
  • /subfolder/sites/all/modules/mymodule/index.php

The latter could (POSSIBLY) be an install of drupal which happens to be nested 6 levels down.

This problem is unlikely and very edge case but I cant help but think there must be a very elegant solution for this!

WiseTeck’s picture

Yeah, got your point. The ambiguity between

  • /subfolder/index.php
  • /subfolder/sites/all/modules/mymodule/index.php

could be solved by using base_path()

If /subfolder/sites/all/modules/mymodule/index.php would be a nested 6 level drupal setup then the base_path would be '/subfolder/sites/all/modules/mymodule/'

so the test should be


if ($_SERVER['SCRIPT_NAME'] != base_path().'index.php') return false;

nicholasThompson’s picture

WiseTeck... Not quite... base_path() simply represents the path that the script was called in. It could be root or it could be a module.

I believe the way the Ad module gets around this is to, upon install, store in a variable the location of module itself. This allows it to compare the current URL to the "known" module location.

I'm wondering if this could be a feature of the new globalredirect admin I've added to 5.x-dev... By default the ONLY URL which should have redirects is base_path() . 'index.php' and this is decided upon module install/update. In the admin area, the site admin can control the paths which ARE allowed to be redirected upon...

gielfeldt’s picture

what about storing the base_path() during installation of the module?

like this:
variable_set('globalredirect_base_path', base_path());

and then in the hook:
$base_path = variable_get('globalredirect_base_path', base_path());
if ($_SERVER['SCRIPT_NAME'] != $base_path.'index.php') return false;

anyone see any problems with this?

nicholasThompson’s picture

gielfeldt: Seems like a sensible solution...

David Lesieur’s picture

robbin.zhao’s picture

this patch works well in the most cases,
but how about the issue,

* /subfolder/index.php
* /subfolder/sites/all/modules/mymodule/index.php

any good solution for this?

I installed my drupal in a subfolder, and I used the .htaccess to redirect to this subfolder, so it looks like in the root folder.

like this:

/.htaccess (redirect to subfolder)
/subfolder/index.php

but the url still looks in root folder, such as my domain, http://www.joke4me.com/, not in www.joke4me.com/drupal,
but actually, I installed the drupal in /drupal, so the global redirect didn't work for me.

Finally, I used the hard code on the line
if ($_SERVER['SCRIPT_NAME'] != $GLOBALS['base_path'] .'index.php') return FALSE;
to
if ($_SERVER['SCRIPT_NAME'] != '/drupal/index.php') return FALSE;

now, works well.

anybody have a good solution?

tharrison’s picture

I have a patch which is an update to the module that solves this problem (I believe) in an effective manner.

The basic problem appears to be that there is no deterministic method to determine the directory in which Drupal is installed. This seems hard for me to believe, but I have not found any method. The current module implementation uses base_path() -- this will fail if Drupal is installed in a subdirectory and the document root directory has an .htaccess rule that directs Drupal requests to the drupal install directory.

The proposed solutions above use $_SERVER['SCRIPT_NAME'], and this will work in almost all cases, since in the Drupal context it will be index.php ... but not quite always, as noted in comments above. So my proposed alternative gets around this problem as follows:

  • Use the $_SERVER['SCRIPT_NAME'] value during module installation to set a variable, the path, as proposed in #4
  • Reference the variable at runtime to build a valid path
  • Allow editing of the path variable through the admin interface, e.g. in case the Drupal install location changes
  • Add some validation that the path is properly formed and exists on the server

Far from rocket science, but for those of us who have (or had) valid reasons to install Drupal somewhere other than the document root, this solution works, is lightweight and should work in the more usual case where Drupal is installed in docroot. (Disclaimer: I have not tested this in the normal case, beyond a code inspection to verify that the case is covered properly).

peter.walter’s picture

Version: 7.x-1.x-dev » 6.x-1.2
FileSize
3.8 KB

I too have Drupal installed in a subdirectory with a .htaccess redirecting to it from the root and found the GR was not doing anything at all.

I confirm that code (based on #8) works with 6.x-1.2 for my use case - see attached patch file.

TommyK’s picture

[edited]

Upon trying the patch in #8 above a second time, I got it working.

Even though in the new Global Redirect configuration settings under Drupal Base Path it looks like the proper path is in there by default, one must click "Save configuration" in order to make it work.

velo_dev’s picture

The patch worked for me as well. Thanks.

nicholasThompson’s picture

Title: not working when drupal is installed in a subdirectory » not working when drupal is installed in a subdirectory - base path config option patch
Version: 6.x-1.2 » 6.x-1.x-dev
Status: Needs work » Needs review
SeanBannister’s picture

sub

portulaca’s picture

subscribing

jspotswood’s picture

I have also tested the fixed detailed in #8 for a Drupal install in htdocs\example.com\drupal with example.com used as the way to access the website. I also did a test with example.com/drupal to access the website. Both worked.
The key thing to do as instructed in #10 and save the configuration of Global Redirect module, before trying the new feature.
Looking forward to thing coming out in a new patch. :)

jspotswood’s picture

I should also add that, ideally this new variable should be eventually moved into the Drupal core, probably in the settings.php file. Surely there must be other modules which need to know this information.

jspotswood’s picture

Sorry for the successive postings regarding this issue. However I like to test things offline or even do large changes to a website using a local copy and then push it to the active website. With this new variable, it is now slightly install specific. While the explicit install directory is not listed, if one does not have exactly the same setup on a local version of the website to that of the live site, there are problems when using a backup of one on the other. Note that this problem can be worked around with the local version of the website made to look similar to the live one, but there can be limitations.
So back to my previous comment, this new variable should really get moved to the settings.php. It seems to be the local and correct place for it.

brad.bulger’s picture

when settings.php is being processed, are there cases where $_SERVER['SCRIPT_NAME'] would either be undefined or not be the desired Drupal root index.php file? or where the directory component of SCRIPT_NAME would not be a correct value for globalredirect_base_path?

i get the reasoning behind the path (i think) and it looks right to me, but as others above have said, i move copies of my site around quite a bit, and try to keep literal file paths out of the config files. but it seems that it would work to put $conf['globalredirect_base_path'] = dirname($_SERVER['SCRIPT_NAME']); into settings.php.

i am not sure how to test the case where a module contains an index.php file, though.

AdrianB’s picture

Subscribing.

nicholasThompson’s picture

I believe the way some modules get around this is, if the script is called index.php, they look for an "includes/bootstrap.inc" file relative to the current file. If that fails, they traverse up the path looking for a folder containing index.php AND a relative subfolder containing bootstrap.inc.

This shouldn't be too "heavy" to add as a variable. Maybe one which gets refreshed on a cache rebuild? (in which case, it could be a cached variable in the {cache} table?)

gooddesignusa’s picture

the patche from #9 seemed to work but not for the alpha version. Any chance we can get a re-roll? Extended Path Aliases needs the alpha version of GR to work.

Anonymous’s picture

subscribing/bookmarking

locomo’s picture

sub

anselm13’s picture

sub

tfo’s picture

+1

Deg’s picture

sub

michaelgiaimo’s picture

sub+

bartoll’s picture

sub+

Status: Needs review » Needs work

The last submitted patch, globalredirect-6.x-1.2-304025.patch, failed testing.

TommyK’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

Attached is a patch where I attempted to update the previously submitted patches to the latest dev (2011-Dec-30). I needed this patch and the latest version of the code, like comment #21, to work with another module; Sub-path URL Aliases in my case.

I really didn't know what I was doing, but it seems to work for me. Any comments or constructive criticism are welcomed. I feel like I learned something by doing this at least. =)

You can probably use git apply if you have cloned globalredirect, but if you have just downloaded the latest dev, you'll probably have to use patch -p1 < path/to/file.patch. That's been my experience so far (see helpful investigation at http://data.agaric.com/git-apply-does-not-work-from-within-local-checkou...).

Status: Needs review » Needs work

The last submitted patch, globalredirect-base_path_settings-304025-21.patch, failed testing.

m.stenta’s picture

Updated patch (based off of 2.15 release):

1. Moved the default base_path variable logic from the admin form builder function into function _globalredirect_get_settings(), which is called at the beginning of the admin form builder to load all settings.
2. Removed the changes to install.php. This update hook has already been released, so we would need to create a new hook_update_6002, but... The only users who have the globalredirect_base_path variable in their systems are those who are using the patch in this issue, since it was never included in the official repository. So we can leave it up to them to renter the setting, or just let the logic in _globalredirect_get_settings() figure it out.
3. Added an esleif to the form submit function that saves the settings... because that function casts all values as integers, which breaks string settings like base_path. I think ultimately the globalredirect_settings_submit_save() function will need to be reworked a little to allow other types of settings... but that should probably be a separate issue from this one.

m.stenta’s picture

Status: Needs work » Needs review
fabio84’s picture

FileSize
415 bytes

I did some search and come up with a simpler solution that should put an end to this bug.

At first, IMHO if a module has an index.php that should be called via http and must bootstrap Drupal, probably this module is badly designed. Why don't just use a path defined via hook_menu()?

However, Drupal relies on the current working directory to include module files, so if you still think you have to bootstrap Drupal from an external .php file, you have to put this file in the Drupal directory, or do somewhere a chdir($my_drupal_directory), otherwise you can't include module files with module_load_include().

I just RTFC of this "ad" module (I never heard of it before I read #205810: conflicts with ad.module ), and in version 5.x-1.3 (the version for which the conflict with globalredirect was reported) they had the file adserve.php , and in line 290 of adserve.inc the function adserve_include_drupal() (included and called sometimes by adserver.php) does some chdir('..') until it finds ./includes/bootstrap.inc
You can see this at http://drupalcode.org/project/ad.git/blob/5ce2b20938ee9128768c063e27af21... , line 290
It seems also the current version has a similar mechanism: http://drupalcode.org/project/ad.git/blob/71ec49a4e24a7c61f427c28f0dca6a... , line 270

So, how can we fix this globalredirect bug with a simple and possibly elegant solution? I think you should just check

if($_SERVER['SCRIPT_FILENAME'] != getcwd() .'/index.php')

It works for me.

IMHO the case where "someone installs Drupal in a subdirectory" is more frequent than a case where "someone creates an index.php file in some subdirectory that bootstraps drupal, and this mess must work with globalredirect". This bug has been open for too long and should be fixed soon. I often install Drupal in a subdirectory to put other static files outside of the Drupal directory, or to test a new site and switch to the new using .htaccess rules in the webroot directory. Maybe this is not the most elegant solution, however everything always worked except globalredirect. I never had time/motivation to investigate until today, I just assumed globalredirect was bogus, and maybe others think the same.

Status: Needs review » Needs work

The last submitted patch, globalredirect-304025-34.patch, failed testing.

fabio84’s picture

Status: Needs work » Needs review

#34: globalredirect-304025-34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, globalredirect-304025-34.patch, failed testing.

fabio84’s picture

FileSize
416 bytes

Sorry, this was my first patch with git, and it seems #1641758: Eclipse EGit 1.1.0 and 1.3.0 creates corrupt patches without LF in last line, 1.0.0 works with bot, so let's try again

fabio84’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, globalredirect-304025-38.patch, failed testing.

fabio84’s picture

Status: Needs work » Needs review
FileSize
436 bytes

Maybe I have to normalize the paths with realpath() to pass the test?

  if (realpath($_SERVER['SCRIPT_FILENAME']) != realpath(getcwd() .'/index.php')) {
    return FALSE;
  }
kemitix’s picture

The patch in #41 works for me. Thanks.

udijw’s picture

patch in #41 works for me too. thanks X2

Jarode’s picture

Status: Needs review » Reviewed & tested by the community

Awesome ! It's been 5 years this issue has been reported and NOTHING ! Not yet fixed...
5 YEARS !!! OHHH MY GODDDD ! Unbelievable ! 8-O

Works like a charm ! Please apply this patch now ! Thank you all.

( 5 years... )

nicholasThompson’s picture

I have pushed the patch from #41 into 6.x-1.x-dev.

Before this is closed, could we do two things:
1) We need to patch this for 7.x
2) We need some test cases to prove it works. How can we replicate the bug? Could we include a bootstrap file with globalredirect (maybe which only works if a flag is set in settings.php, for security reasons)...

nicholasThompson’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
siramsay’s picture

just patch 6.x-1.5 and it worked on redirecting node/111 to its alias and removed the trailing /

uncanny timing really, but just came across this bug last week and this bug report today

thanks

phiscock’s picture

I have attempted to create a patch for version 7.x-1.x-dev, which is attached for testing in the hope it helps some people.

However it didn't solve the problem for me as I seem to be suffering from the problem in http://drupal.org/node/1805616.

DaneMacaulay’s picture

patch tested and works as expected.
rtbc

janbrulc’s picture

38: globalredirect-304025-38.patch queued for re-testing.

The last submitted patch, 38: globalredirect-304025-38.patch, failed testing.

dsdeiz’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.42 KB

Re-rolling. The patch from #48 still applies although only had a trailing white space.

DamienMcKenna’s picture

DamienMcKenna’s picture

I reopened #2285209 because it deals with the tests specifically.

wojtha’s picture

+++ b/globalredirect.module
@@ -324,7 +324,7 @@ function _globalredirect_is_active($settings) {
-  if ($_SERVER['SCRIPT_NAME'] != $GLOBALS['base_path'] . 'index.php') {
+  if (realpath($_SERVER['SCRIPT_FILENAME']) != realpath(getcwd() .'/index.php')) {

I can confirm that solely this line fixed my issue in Drupal 7 which is installed at path "/www/domains/{domain.tld}" but DocumentRoot is set to "/www/".

fabio84’s picture

Title: not working when drupal is installed in a subdirectory - base path config option patch » not working when drupal is installed in a subdirectory
Issue tags: +Barcelona2015
FileSize
485 bytes

Recently I tested the same one-line solution on a couple of web site we recently updated to Drupal 7

Right now I'm at DrupalCon Barcelona2015 and I'd like to submit the patch for 7.x-1.x

I think the title of the issue is misleading, because with my patch you don't have to add an option to set the base path, so I'm removing the last part of the title

Chris Matthews’s picture

Assigned: nicholasThompson » Unassigned

The 3 year old patch in #56 to globalredirect.module applied cleanly to the latest 7.x-1.x-dev, but still needs community review & testing.

cmseasy’s picture

Using this patch for years on different sites without issues: please commit

Chris Matthews’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, changing the status to RTBC so it has a chance to be committed.