I'm using Global Redirect for all sites I've built, and on one site it's giving me a bit of trouble. The site is multilingual and when I've tried to put it behind a CDN i discovered a problem.

For any request regardless of if it is a file or a page that does not exist, it always first makes a redirect with a prefix of /sv/ (the default language). This has the effect that when a new image style should be generated, the request returns a redirect, and that redirect is then cached in the CDN instead of the image itself.

It works by default because redirects are not cached, so the next request goes for the newly generated image, but I want to be able to cache them directly. And I want to get rid of the unnecessary redirect and extra request it triggers. As a solution I've added this little statement to the main redirect init function. It stops Global Redirect from doing anything if the request looks like a file.

// Do not redirect requests for files
if ( preg_match('/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp|pdf)$/i', $request_path) ) {
  return;
}

It seems to work well.

Another related issue is when spiders index your site for old or changed nonexistant URLs, and all they get are 302s to the same URL prefixed with language. Then on that URL they are finally given a proper 404. Would it be possible to somehow detect if the final URL will actually lead somewhere and if it doesn't return a proper 404 directly?

CommentFileSizeAuthor
#13 globalredirect-no-redirect-for-static-files-2223181-12.patch3.11 KBvincenzodb
#10 globalredirect-no-redirect-for-static-files-2223181-10.patch2.72 KBsi.mon
#9 globalredirect-no-redirect-for-static-files-2223181-9.patch2.24 KBsi.mon
#2 globalredirect-no-redirect-for-files-2223181-2.patch1.27 KBsi.mon
#1 globalredirect-no-redirect-for-files-2223181-1.patch604 bytessi.mon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

si.mon’s picture

si.mon CreditAttribution: si.mon commented
Version: 7.x-1.5 » 7.x-1.x-dev
Category: Feature request » Bug report
FileSize
globalredirect-no-redirect-for-files-2223181-1.patch604 bytes

Same issue here. I might be wrong but to me redirecting static files looks like a bug. If not, an option to choose whether to redirect files or not might be nice.
Patch is attached, would be nice to see it commited.
Thanks !

si.mon’s picture

si.mon CreditAttribution: si.mon commented
FileSize
globalredirect-no-redirect-for-files-2223181-2.patch1.27 KB

Just added a second patch allowing to make this configurable through the admin interface. Users can now choose if they want globalredirect to redirect files or not.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

A similar problem happens if your system exposes service requests.
All requests then also receive a redirect to the default language prefix. Even worse with language domains...
This leads to a performance regression and we needed to add the language prefix to services clients to fix the situation.

si.mon’s picture

si.mon CreditAttribution: si.mon commented
Title: Do not redirect requests for nonexistant files (jpg, png, pdf, etc) » Do not redirect requests for static files

Could someone please review the patch ? Would be great to see this RTBC and commited as it seems to be a common issue.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs review » Needs work

Note that these extension patterns are usually based in .htaccess that is subject to change on different environments.
So these patterns should not be detected with a static string. At least an override through variables (with override option from config or even UI) should be possible.

Also, we currently focus on file endings / name match.
A name based matching could also be used for services pathes instead of static files only. Or we need a followup issue for that.

We could also detect public / private files pathes instead of file endings...?

si.mon’s picture

si.mon CreditAttribution: si.mon commented
Status: Needs work » Needs review

Miro, I do agree that this could be even better. But what you are describing are new features. This issue is about a bug preventing the correct behavior of a Drupal website in certain conditions. For instance, my websites can not use CDN's far-future option because of this, and that's a real blocker.
What I would suggest is to get this issue fixed asap and then open a feature request in order to add all those fancy features you are talking about. I'd be glad to give you a hand on this. If you agree, could you review the patch ?
Thanks !

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Agree about separating the features.
A static hardcoded list of file endings just feels totally wrong to me. I still recommend you add a variable to (optionally) containing the exact patterns.

valderama’s picture

valderama CreditAttribution: valderama commented

Please note that the current behavior of globalredirect also breaks the on-demand generation of CSS as provided by the Advanced CSS/JS Aggregation module (https://www.drupal.org/project/advagg).

Because of that I decided to go with the provided patch - which works.

si.mon’s picture

si.mon CreditAttribution: si.mon commented
FileSize
globalredirect-no-redirect-for-static-files-2223181-9.patch2.24 KB

Adding new patch allowing the static file extentions pattern to be changed in the admin interface.
Please review and RTBC if it looks ok :)

si.mon’s picture

si.mon CreditAttribution: si.mon commented
FileSize
globalredirect-no-redirect-for-static-files-2223181-10.patch2.72 KB

Adding new patch (based upon #9) which fixes some PHP notices because of missing tests.
Please review and RTBC if it looks ok :)

SocialNicheGuru’s picture

SocialNicheGuru CreditAttribution: SocialNicheGuru commented
Issue tags: +CDN, +globalredirect, +redirect

If using the http://drupal.org/project/cdn module should the static files mimic what is placed on the cdn config page

kpv’s picture

kpv CreditAttribution: kpv commented
Status: Needs review » Reviewed & tested by the community

confirm patch working, code seems to be ok

vincenzodb’s picture

vincenzodb CreditAttribution: vincenzodb as a volunteer commented
FileSize
globalredirect-no-redirect-for-static-files-2223181-12.patch3.11 KB

Patch that apply to 7.x-1.5 version

vincenzodb’s picture

vincenzodb CreditAttribution: vincenzodb as a volunteer commented
BarisW’s picture

BarisW CreditAttribution: BarisW at LimoenGroen commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -globalredirect, -redirect

Please only create patched against the dev version. This needs a re-roll. Also:

+++ b/globalredirect.admin.inc
@@ -104,6 +104,31 @@ function globalredirect_settings() {
+  $form['settings']['comment_to_node'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Redirect Comments To Node'),
+    '#description' => t('If enabled, any request to a comment URL will redirect to the associate node.'),
+    '#default_value' => $settings['comment_to_node'],
+  );

This shouldn't be part of this patch.

One more thought; why are we checking on file extension? Wouldn't it be better to check if the request starts with the the public or private files folder?