Remove symlink creation. Let each path have own file

Yes, a setting for whether to use symlinks, or not, does sound pretty all right in principle. The problem, however, is that Boost presently really relies on the presence of symlinks, and adding non-trivial special cases at a good number of places in the code is akin to obfuscation.

I think it would be preferable that your patch would try to abstract out the OS-specific parts somehow; for instance, you could first look through the code to find out which functions rely on symlinks, and then separate those bits out from boost.api.inc into, say, boost.unix.inc and boost.windows.inc, or something like that. Then we could make do even without an explicit setting, just checking the current operating system in order to see which file we should load into boost.api.inc.

If you do proceed on this front, please make sure to adhere to the existing coding conventions in the code, and submit your results here in the form of a patch. Also, please be aware that I don't myself have any Windows machines available for testing, so in practice you yourself would need to support and further develop the Windows bits as necessary; if you're willing to do that, I would make you a co-maintainer of the Boost project here on drupal.org so that you can take ownership of any Windows-specific issues and such.

Hi Daniel,

Currently at DrupalCon, but will try and review your patch next week when I get back home.

windows does actually support symlinks but only for directories - not files. so one option would be for boost to produce many directories all with a single index.html inside. this is a bit less clean than the current solution but straightforward windows compatibility using same code is very nice.

we would make a wrapper around the symlink() function in php so it shells out to the 'junction' program in windows which created symlinks. see _symlink() in the comments at http://us.php.net/symlink for an example wrapper. the junction program for windows is available for free at http://www.microsoft.com/technet/sysinternals/FileAndDisk/Junction.mspx

Moshe, that's a very interesting idea. I'd love a patch that worked the way you describe.

Hmmm. Caching to alias-named files instead of using symlinks would take care of the "too many files in your /node/ directory, you're killing our server" problem, here: http://drupal.org/node/171444

I think it's more elegant (it removes one level of complication), but it's possible that I think so because I'd need to get around the 171444 problem, too; I have 30+k nodes.

From bjaspan:

It seems to me that the symlink code is unnecessary complexity on Unix and extra logic to work around not having symlinks on Windows is even more unnecessary complexity. Just write the HTML out to whatever path was requested! So we end up with two copies of some pages if people go out of their way to access node/nid. So what? Disk is cheap.

I agree so I set this issue to 'needs work'. Feedback welcome.

This fixes Windows compatibility so thats why we keep discussion in this issue.

Note that globalredirect.module is useful for assuring that noone goes to node/nid URL. I see little value in the symlinks considering how popular that module is.

Here is my patch to stop using symlinks. I only stop calling _boost_symlink(), I haven't actually removed the helper function yet.

If one were to use this patch this means i18n should now work as well, correct?

@justafish: Yes! i18n works with this patch.

fixes my issues with i18n as well.

i also feel that the problems introduced by symlinks wrt. interop with other modules outweighs the relatively minor benefits.

i take it back, i still have an i18n issue.

let's say that default language is en
if the user requests a page like http://www.example.com/de
then boost_set_cache is called with $path=''
and drupal_get_path_alias('') returns 'de'
boost creates two cached files: index.html and de.html

now, if a user comes in with no default language setting, they will receive the cached german index.html

however, i don't think that invalidates this patch. this issue is about disabling symlinks, and the patch works perfectly for that. i18n support is a separate thing.

Daniel,

Is this module working for windows? What changes did you have to make in regards to clean urls? We are using ISAPI mod rewrite 3 and I am not sure how to change the configuration file to make this module work on a Windows 2003 server. Help!

-Drew

Hi,
Will you apply this patch if i provide you with windows and linux compatible patch?

Hi,

Tried the patch #14 on the D6 alpha and it looks ok, does anyone know if I can use the same htaccess rules with IsapiRewrite3? If not any ideas.

PS: I'll try later myself :p

small update, i just saw this in my recent logs: mysqli_real_escape_string() expects parameter 1 to be mysqli, null given in C:\sites\drupal6\includes\database.mysqli.inc on line 323. No idea if it's related or not.

It's related and very strange, if I add the watchdog line it's working, if I remove the line, I get the error above and $alias = $path. The strange thing is that it's working all over the site, is it possible that the $active_db isn't known yet?

It's working using the watchdog line, 'cos dblog_watchdog($log = array() is setting calling db_set_active();

watchdog('attiks', 'ok');

  $alias = drupal_get_path_alias($path);
  $path = drupal_get_normal_path($path); // normalize path

Final code

  db_set_active();
  $alias = drupal_get_path_alias($path);
  $path = drupal_get_normal_path($path); // normalize path

I created a separate issue for this: http://drupal.org/node/326241

Next step was to change .htaccess, I added the following (I added the U to the RewriteRule so the original path shows in the logfiles!) and after an iisreset all is working, I get the cached page in 70ms :D

  RewriteCond %{REQUEST_METHOD} ^GET$
  RewriteCond %{REQUEST_URI} ^/$
  RewriteCond %{QUERY_STRING} ^$
  RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
  RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}/index.html -f
  RewriteRule ^(.*)$ cache/%{SERVER_NAME}/index.html [U,L]
  RewriteCond %{REQUEST_METHOD} ^GET$
  RewriteCond %{REQUEST_URI} !^/cache
  RewriteCond %{REQUEST_URI} !^/user/login
  RewriteCond %{REQUEST_URI} !^/admin
  RewriteCond %{QUERY_STRING} ^$
  RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
  RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI} -d
  RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI}/index.html -f
  RewriteRule ^(.*)$ cache/%{SERVER_NAME}/$1/index.html [U,L]
  RewriteCond %{REQUEST_METHOD} ^GET$
  RewriteCond %{REQUEST_URI} !^/cache
  RewriteCond %{REQUEST_URI} !^/user/login
  RewriteCond %{REQUEST_URI} !^/admin
  RewriteCond %{QUERY_STRING} ^$
  RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
  RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI}.html -f
  RewriteRule ^(.*)$ cache/%{SERVER_NAME}/$1.html [U,L]

@Arto - I think the case for simply dropping the symlink code is compelling. What do you think? Others?

One more point - Google strongly discourages serving duplicate content on two different URLs. All the dupes should 301 redirect to the authoritative location. I recommend removing symlink code and letting Path redirect module do its job (thats the one that pathauto recommends). Drupal should never be handing out the unaliased urls so performance is not relevant here.

Confirming that patch in #14 makes Boost work on Windows. Nice!

Does it work on Windows with Drupal 5? We would love to try this module to get a performance boost on our site as it gets a lot of traffic and is a bit slow. So if we install the current module and apply the patch we would then just need to apply the rewrite rules in #23 (in IsapiRewrite) to get it working? Let me know, thanks!

-Drew

When I was confirming, I was confirming that it works on Drupal 5, using Apache on Windows. I cannot tell whether it works with IIS. Please test yourself, and open a separate issue for IIS compatibility if it does not work. Thanks.

See #21, #22, #23: it's working on IIS 6 using drupal 6, I don't see any reason why it shouldn't work with D5 since the hard part is about .htaccess.

You need the patch in #14, the rules in #23 are a copy of the included rules (see the readme).

Ok, I have it mostly working I think but it is still not pulling the pages correctly from the cache directory. I can see that they are being created in there but anonymous page loads do not have the Page cached by Boost at.... comment at the bottom. I have a bunch of rewrite rules with IsapiRewrite3 for clean URLs and also to force a redirect for tribalectic.com/xxxx to www.tribalectic.com/xxx. Is there any order in which the rules need to be (in other words do these rules need to be first, last, etc.)? Any help would be much appreciated, here are our current rules:

RewriteEngine on
RewriteCond %{HTTP:Host} ^(?:www\.)?tribalectic\.com$
RewriteBase /Drupal/
RewriteCond %{HTTPS} (on)?
RewriteCond %{HTTP:Host} ^(?!www\.)(.+)$ [NC]
RewriteCond %{REQUEST_URI} (.+)
RewriteRule .? http(?%1s)://www.%2%3 [R=301,L]

RewriteCond %{REQUEST_METHOD} ^GET$
RewriteCond %{REQUEST_URI} ^/$
RewriteCond %{QUERY_STRING} ^$
RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}/index.html -f
RewriteRule ^(.*)$ cache/%{SERVER_NAME}/index.html [U,L]
RewriteCond %{REQUEST_METHOD} ^GET$
RewriteCond %{REQUEST_URI} !^/cache
RewriteCond %{REQUEST_URI} !^/user/login
RewriteCond %{REQUEST_URI} !^/admin
RewriteCond %{QUERY_STRING} ^$
RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI} -d
RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI}/index.html -f
RewriteRule ^(.*)$ cache/%{SERVER_NAME}/$1/index.html [U,L]
RewriteCond %{REQUEST_METHOD} ^GET$
RewriteCond %{REQUEST_URI} !^/cache
RewriteCond %{REQUEST_URI} !^/user/login
RewriteCond %{REQUEST_URI} !^/admin
RewriteCond %{QUERY_STRING} ^$
RewriteCond %{HTTP_COOKIE} !DRUPAL_UID
RewriteCond %{DOCUMENT_ROOT}/cache/%{SERVER_NAME}%{REQUEST_URI}.html -f
RewriteRule ^(.*)$ cache/%{SERVER_NAME}/$1.html [U,L]

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_FILENAME} !\.(s?html|css|js|cgi|asp)$
RewriteRule ^(.*)$ index.php?q=$1 [L,QSA]

Does the fact that our Drupal is in a sub-directory called "Drupal" matter? Thanks!

-Drew

I see the problem actually. It is looking for the cached file in...

D:\tribalectic\Drupal\cache/www.tribalectic.com/0/Drupal/articles/ear-body-piercing-healing-and-afte...

but the cached file is in...

D:\tribalectic\Drupal\cache/www.tribalectic.com/0/articles/ear-body-piercing-healing-and-aftercare-i...

Basically, when boost is caching files it is not taking into account the sub-directory. Is there a way in rewrite to remove the "Drupal/" from {REQUEST_URI}? Or better yet, a way for boost to use the sub-directory when creating cached files?

-Drew

How can i use this patch for sites with querystrings?

I think that from the moment there's a querystring in the clean URL, the caching mechanism isn't used and the request is allways send to drupal for processing. For example if you use search you want drupal to process the request otherwise you'll end with stale records.

Correct. Also, the 2nd line of INSTALL.txt explicitly mentions:

Drupal's clean URLs MUST be enabled and working properly.

I build my own content type, which uses a pager. The first page (e.g. node/123) is written by Boost, but the other pages (e.g. node/123?page=0.2) aren't be written by Boost. I rewrite the "node/123?page=0.2" to "node/123_page_2" via htaccess.
Can you help me?

Agree with #12, boost must cache queries as is, just like the drupal core caching system does. There would be no problems with symlinks, i18n, etc.
The patch for 6.x-1.x-dev, needs testing:

New version. Forgot to change boost_block() function

Testing your patch (btw, thank you!!) , I am facing a strange (and very odd) behaviour: boost seems to work fine for every page, -although I have not intensively tested every situation-, except for the frontpage, the one which has no language prefix (frontpages with language prefixes are loading ok from the cache and being created with the correct names), and no uri after the domain name.

Depending on the used browser, the error is different, all of them get stuck -but connected to the server- until a timeout response. Firefox says that the response is being redirected in an endless loop. I can confirm it is not a problem with .htaccess, as i can solve the problem just disabling boost while leaving .htaccess untouched.

I have done several tests, and it happens even when is not included in the list of cacheable pages, so it looks like somewhere in the boost flow the case where $_REQUEST['q'] (or $GET['q']) is empty is not correctly processed.

I have reviewed the code -up to my limited coding knowledge- but haven't found where the problem is. Any clues?

I have managed to solve the problem. I don't know much about drupal internals and $_REQUEST['q'] usage, but it seems that setting it to 'index' when is empty breaks something in the drupal processing of the page (although could be related to my drupal install)

My proposal is a bit dirty, (sorry, I am not a coder), so maybe someone finds a better solution. Once the pach in #38 was applied, I modified the function boost_init() to not alter $_REQUEST['q'], and directly process the page in case we are dealing with '/' . Where the function stands:

    // We only serve cached pages for GET requests by anonymous visitors:
    else if ($_SERVER['REQUEST_METHOD'] == 'GET') {
      // We use $_REQUEST['q'] because drupal_init_path() had already changed $_GET['q']
      if (empty($_REQUEST['q'])) {
        // special handling for Drupal's front page
        $_REQUEST['q'] = 'index';
      }
      // Make sure no query string (in addition to ?q=) was set, and that
      // the page is cacheable according to our current configuration.
      if (count($_GET) == 1 && boost_is_cacheable($_REQUEST['q'])) {
        // In the event of errors such as drupal_not_found(), GET['q'] is
        // changed before _boost_ob_handler() is called. Apache is going to
        // look in the cache for the original path, however, so we need to
        // preserve it.
        $GLOBALS['_boost_path'] = $_REQUEST['q'];
        ob_start('_boost_ob_handler');
      }
    }

I changed it to:

   else if ($_SERVER['REQUEST_METHOD'] == 'GET') {
      // We use $_REQUEST['q'] because drupal_init_path() had already changed $_GET['q']
      if (empty($_REQUEST['q']) && boost_is_cacheable('index')) {
        // special handling for Drupal's front page
        //$_REQUEST['q'] = 'index';  do not change this variable, is breaking something
        $GLOBALS['_boost_path'] = 'index'; 
        ob_start('_boost_ob_handler'); 
      }
      // Make sure no query string (in addition to ?q=) was set, and that
      // the page is cacheable according to our current configuration.
      else if ((count($_GET) == 1 && boost_is_cacheable($_REQUEST['q']))||( $case_root == 1 && boost_is_cacheable('index'))){
        // In the event of errors such as drupal_not_found(), GET['q'] is
        // changed before _boost_ob_handler() is called. Apache is going to
        // look in the cache for the original path, however, so we need to
        // preserve it.
        $GLOBALS['_boost_path'] = $_REQUEST['q'];
        ob_start('_boost_ob_handler');
      }
    }

I am not sure if this is going to broke something, but is working for me. Sorry for not uploading a patch: I am not sure what I am doing, especially when patching things, and also I have my files modified to support gzip compression, so I don't know what to patch against what :-)

Anyway, if someone finds it useful, this is my boost.module file, that provides the magic of gzip compressed multilingual static caching!! (for a recently upgraded D6 install) :-))
Should work with boosted1.txt file Nagibator himself uploaded here some time ago

thank you again por the patch, Nagibator!!!. Seems we are boost fans :-))

pedropablo, thanks for pointing this out. I don't have this problem, maybe one of installed modules works with $_REQUEST.
Here's a new version, which does not change $_REQUEST. Please check whether it works.

Perfect, a much much more cleaner code... :-)
Works with no problem at all. thx

I think it would be a good idea to merge this patch with the querystring patch. What do you think about this?

The latest patches in this thread are for D6, but I wanted to confirm that for D5 the patch in #14 works great.

This may just be my confusion, but is this patch actually allowing the Global Redirect functionality to work? So far as I can tell, now my pages are accessible at two different locations: [page-alias].html and node/[nid].html. Won't that cause the Google "sandbox" effect that the Global Redirect module is supposed to prevent?

For the Global Redirect module to work properly, wouldn't we have to make it so that the node/[nid] URL paths did a 301 redirect to cache/[site-name]/0/[page-alias].html?

Thanks for this patch - I just want to understand further the implications of it before I implement Boost on our site.

So far as I can tell, now my pages are accessible at two different locations: [page-alias].html and node/[nid].html. Won't that cause the Google "sandbox" effect that the Global Redirect module is supposed to prevent?

Only if you are doing something silly like start posting external links to your sites cache files. Otherwise there is no way for Google to find the cached .html file. With the Boost .htaccess rules these static pages get served at the original URL, so the process is completely transparent.

Global Redirect is only useful if you are highly concerned about the last minutiae of SEO, and you have _many_ external links that point to different access points of the same node (i.e. there are links in the wild to /node/1 and ?q=node/1 and /some/alias/to/node/1 ). For the majority of sites this will never happen.

Thanks for the clarification. So if Global Redirect is only useful in special cases, is there a need for this patch on a Unix server? I don't want to create 2 copies of each page on my site unless there is some benefit for doing so.

I added the patch and it was caching search pages. I excluded this with the following lines:

// Don't cache search pages (nader)
if (preg_match('!^search/node!', $normal_path))
return FALSE;

An alternative patch for d5. Just 'cause. Note: this is also based on the backported querystring-aware patch from #182687: Cache pages with query strings.

At the risk of asking a silly question ...

Where does this go?

I have LOTS of nodes ... 640,000 and the nodes folder in cache is EXPLODING.

I think the best thing to do is to subdivide the nodes directory and rewrite it as follows
all ids < 10000 goes in the the node directory
for ids > 10000 make separate directories as such
node/1/9999
node/2/9999
....
node/59/9999

and figure out a way to make a new rewrite rule :p

640,000 files in one directory is not an issue, at least on Linux/Unix-based operation systems.

In any case, that's a completely separate issue, so please create a new one, but search the queue for existing first.

I actually did create a new issue already ... http://drupal.org/node/410730

I was following mikeytown2's suggestion of using this fix to avoid the problem.

What I was wondering was where do I apply this fix so that it will work? Or should I just wait for the other fix to occur?

thanks!

patch needs to be re-rolled against latest 6.x dev. http://drupal.org/cvs?commit=193760 breaks this in the 2nd and 10th Hunk; #8 suspect as well. 2nd one is the tricky one.

trying to re-roll the #174380-41: Remove symlink creation. Let each path have own file patch but needing some advice:

(patch disection by hunk):

1. trivial - adds

+//  exit();
+

removed this

2.1.

+      // We use $_REQUEST['q'] because drupal_init_path() had already changed $_GET['q']
+      $boost_path = $_REQUEST['q'];
+      if (empty($boost_path)) {
+        // special handling for Drupal's front page
+        $boost_path = 'index';
+      }

is this STILL the right thing to do? Using this $boost_path tmp var? The rest of the 2nd hunk is just replacing $_GET['q'] with this tmp var...

3. and further

- if (!empty($user->uid) && boost_is_cacheable($_GET['q'])) {
+ if (!empty($user->uid) && boost_is_cacheable($_REQUEST['q'])) {

is the replacement of $_get with $_request the right thing to do? I guesss so. (cause when i dumped $_GET and $_REQUEST - in REQUEST I got de/node/65 while in $_GET - only node/65 for the 'q' key)

Later hunks to follow after we agree on these matters.

This should make it so the $boost_path variable isn't needed. Untested code below.

$GLOBALS['_boost_path'] = (empty($_REQUEST['q'])) ? 'index' : $_REQUEST['q'];

Does using abbreviated IF/ELSE meet drupal's coding standards?

Replacing GET with REQUEST is probably the right thing to do because drupal_init_path() changes the GET variable. There is even a report of this fixing an error pages are cached, but not retrieved - #37.

Scratch the above. Useage of $boost_path is needed because of if (count($_GET) == 1 && boost_is_cacheable($boost_path)) { .

1. about $_GET and $_REQUEST: drupal_init_path() changes $_GET['q'] only by trimming / around it and then if it is an alias - makes it in the form of node/56 ... So using $_REQUEST['q'] enables boost to cache aliased pages ...

2.1.

$GLOBALS['_boost_path'] = (empty($_REQUEST['q'])) ? 'index' : $_REQUEST['q'];

I am not sure if the ()-s go around the empty() or around the whole thing :

$GLOBALS['_boost_path'] = (empty($_REQUEST['q'])) ? 'index' : $_REQUEST['q'];

or

$GLOBALS['_boost_path'] = (empty($_REQUEST['q']) ? 'index' : $_REQUEST['q'] );

or no ()-s at all will do fine.

So using the full form :)

2.2. drupal coding standarts say nothing of the short if/else... neither does the php.net site docs (except for some user comments)

2.3. I suppose it iwll be okay to use $GLOBALS['_boost_path'] from the very start and use it instead of $boost_path - like in the attached patch for hunk 2.

YES, after 2-3 hours of manual rerolling and checking - here is the new patch attached. It is against the latest dev checkout from the DRUPAL-6--1 tag.

REVIEW:
I've looked into the new code - esp. made sure the changes dont break anything - like leave stale vars or which func calls which - to be sure that input of one is properly proccessed while feeding the other (this is about the $path var in the code). Also I've commented at some points what has just been removed - for patch readability. Right before commiting we can drop these...

Somebody from the more experienced - please review this.

TESTING:
I got : Clean patching and a working site i18n site with boost. All fine now. Before the site didn't work.

PS: The node/65.html files are no longer created - desired effect or bug? I am ok with it though.

Will test this over the weekend, currently swamped with work. Quick glance at the patch, and it looks like it will work :)

possible expiration issue (not sure if it is an issue at all and whether caused by this patch) - boost cleans the entire cache dir on cron... cross-posting this here from another thread around here...

Tested & Committed. Then I realized that the boost block wasn't working correctly... made a quick fix here
http://cvs.drupal.org/viewcvs/drupal/contributions/modules/boost/boost.m...
Issue revolved around the $GLOBALS['_boost_path'] variable. For some reason, it was not being set when viewing the block. Issue was placement of $GLOBALS['_boost_path'] inside boost_init(). It's now fixed.
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/boost/boost...

quote from mikeytown2: "...Finally roll out the monster patch ..." from http://drupal.org/node/371809#comment-1432366

if all goes well with this patch alone - I wish for a prize for taking the monster down ( free drink or a fancy drupal t-shirt or a poem :) or whatever ) :)

Cheers.

EDIT: I figured out the prize - a new keyboard for my laptop :)

#49
http://drupal.org/node/174380#comment-1344738

Is there a patch for 6.x-1.0-rc2?

@FrontBurner
This was fixed in 6.x-1.0-alpha2

Any development release for 5.x?

@sinasalek
nope #454652: Looking for a co-maintainer - 5.x. Once 6.x-1.0 stabilizes, I'll be working on 7.x.

Closing all 5.x issues; will only reevaluate if someone steps up #454652: Looking for a co-maintainer - 5.x

Reason is 6.x has 10x as many users as 5.x; also last 5.x dev was over a year ago. The 5.x issue queue needs to go.