Set cached pages to more reasonable age.

Adding link to previous discussion on infrastructure list.

well, if the thing is accepted :)

but, the issue is that we need a good mechanism to identify the age of a page. And a page is not only the node proper, quite a few things contribute to the age, and we need to be able to determine which of all should be considered important or not. And keep things simple while doing it.

We can only know all contributing factors just before a page is rendered, when all of the information is assembled - i.e. nodes, blocks, etc... By the way that is valid for all rendering, not only html pages but various feeds and similar animals.

This is the essence of what I was showing to killes earlier.

but, the issue is that we need a good mechanism to identify the age of
a page. And a page is not only the node proper, quite a few things
contribute to the age, and we need to be able to determine which of all
should be considered important or not. And keep things simple while
doing it.

We can have an automatic or semi-automatic way for each piece of content
(e.g. node, comments, blocks, ...etc.) to "push" their date up a "register".

For example.

Module 1 pushes its date 2005-05-01 14:00:00, there is nothing in the register yet, so the common function/hook that does that just places this here.

Module 2 pushes its date 2005-04-29, the common function finds that this is older than what is there, and discards it.

Module 3 pushes date 2005-05-05, the common function picks that up since it is newer.

This process repeats until we end up with the date of the page being the date of the newest "piece" of content displayed on it (e.g. comment, node, ...etc.)

Some stuff has to be exempt from this (e.g. banners), and some can be elective/optional. But for the main content stuff, this overall solution seems workable. The question is: should this be an explicitly called function? If so, then where? Or can it be automated somewhere?

kbahey, my patch implements what you describe, only it isn't complete. I will complete it once we agree the approach is a good idea.

Recent comment blocks, forum topic blocks, news aggregator blocks, popular content
... all have to be sprinkled with drupal_set_cache_date()? Also some blocks look incompatible; the recent poll block shows the votes. We'd need to add timestamps to votes?

This approach looks prone to errors.

 function comment_nodeapi(&$node, $op, $arg = 0) {
   switch ($op) {
     case 'load':
-      return db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
+      $comment = db_fetch_array(db_query("SELECT last_comment_timestamp, last_comment_name, comment_count FROM {node_comment_statistics} WHERE nid = %d", $node->nid));
+      drupal_set_cache_date($comment['last_comment_timestamp']);
+      return $comment;
       break;

This looks wrong. Just because we load a node, doesn't mean we're going to display it ... ?

This patch seems very worthwhile to me. I can think of some sites I work with that have lots of old content, it'd be nice to save all that wasted bandwidth and cpu...

Re: the concerns about blocks updating the cache date:

I think the "date" you're trying to fix made complete sense in the days of static html, but doesn't anymore for many pages, and isn't always respected. For example, I'm pretty sure that most browser's default settings are to reload pages regardless of the date at the beginning of a new session. I guess it's still quite important for images and maybe css files.

My point is that it doesn't need to be 'perfect' in the sense that there could be a different page sent and still you don't want to update the date (the ad banners is the example already cited).

Instead, I think the goal should really be: when has the content changed significantly enough to require a new date? In that context, only modules that are essentially changing the content of the page need to care about this. And maybe it even depends on the particular site? For example, many newspapers run their comments on a separate server and perhaps only include the list of comments via some javascript - in their case, comments don't change the date, and they probably don't want them to. But of course on drupal.org, where comments often have important information on the issue at hand, you would want google to respider a page when it gets a new comment.

With that perspective, I think it's a worthwhile patch. Some administrator options to allow a site to disable the cache-date-update for specific modules would be nice as well.

Part of this bandwith problem can be solved by a sensible robots.txt as well: http://drupal.org/node/75916

Some combination of this patch + a robots.txt would really go a long way in saving bandwith to crawlers.

I gave this one a good testing and it works as advertised. I had to fix one SQL bug. New patch for 4.7 attached.

Yes, various blocks are going to have to set cache age. They can do set age to time() if thats what makes sense. For something like polls, perhaps we bump the last_modified of the node upon each vote. that would solve the need.

I would call the column 'last_modified' and the function drupal_set_cache_last_modified

Looks like a good idea to me, in light of the mammoth resources consumed by crawlers. At leatsd I cannot think of an alternative.

Subscribing to this as it seems interesting.

Whats being done about this and whats the conclusion?

@killes (or anyone else) - make sense to revive this one for D6?

For cached pages, there is a related patch that does better headers for caching.

It is fairly simple, and would have a positive impact on a site.

Check it here http://drupal.org/node/147310

sigh - move to 7

Sigh, indeed. This should have made it into D5 and then D6.

This is an old issue that doesn't have a working implementation... why should it have gotten into d5 and d6? Moshe's update in #16 is also missing the revised patch.

I didn't say an out of date patch should have gotten applied to D5 and D6. I meant that correct, fixed last-changed dates should have been in D5 and D6. This issue and a suggested fix was raised back in 4.7 days, so there was plenty of time to incorporate it into core. Sheesh.

I still think this language is clearer: db column: 'last_modified' and the function: drupal_set_cache_last_modified.

We now have block caching. I think that we can incorporate blocks into this system with just a single change. We don't have to change every block anymore.

I've changed the name of the column on the cache table to last_modified and the function name to drupal_get/set_page_last_modified. The function is used by the cache but also in general for generating pages.

I've changed the name of the column on the cache table to last_modified and the function name to drupal_get/set_page_last_modified. The function is used by the cache but also in general for generating pages.

I'll investicate to coupe it with the block cache.

Ok, patch updated, some bugs fixed.

If a block is loaded from cache, $cache->last_modified is used to set the page's page_last_modified value.

Ok, another update.

Seing this is now a bugfix, I am putting this in the D6 queue again.

If you are a core committer, feel free to move it back.

Relatedly... last night I discovered another benefit of this patch. When you grab an RSS feed from Drupal, the LAST-MODIFIED always has the date you grabbed the feed, which is bad. Feed readers think there is always new content, even if there isn't, so it has to double check everything again and grab the whole feed again.

On patch review... drupal_set_page_last_modified() why do we compare if greater than? What if the last_modified is set to say "10" and then I want to set it to "8" -- it would be less than and fail. Sure that probably won't happen, but wouldn't isset() be better?

Some debug code in there there watchdog('debug', "block cid $block->module $block->delta $cid"); (2 instances)

This looks like it'd have a lot of benefits, I know a load of my traffic is from search engine crawlers. Given m3avrck's review setting to CNW.

m3avrck, if I have some piece of content that was modified a week ago then there should be no way for a further call to set the page age a month old. That's why we never set the time back.

       'created'    => array('type' => 'int', 'not null' => TRUE, 'default' => 0),
+      'last_modified'  => array('type' => 'int', 'not null' => TRUE, 'default' => 0),

That's a code style error...

Added spaces (do not credit me).

Two, two, two! hunks failed...

$ patch -p0 < reasonable_age-65017-37.patch
patching file includes/bootstrap.inc
Hunk #1 succeeded at 566 (offset -1 lines).
Hunk #3 succeeded at 629 (offset 1 line).
patching file includes/cache.inc
patching file modules/block/block.module
Hunk #1 succeeded at 441 (offset 3 lines).
patching file modules/comment/comment.module
Hunk #1 succeeded at 592 (offset 19 lines).
Hunk #2 succeeded at 914 (offset -79 lines).
Hunk #3 succeeded at 1030 (offset 19 lines).
Hunk #4 succeeded at 996 (offset -86 lines).
Hunk #5 succeeded at 1142 (offset 19 lines).
Hunk #6 succeeded at 1325 with fuzz 2 (offset 16 lines).
patching file modules/node/node.module
Hunk #1 succeeded at 974 (offset 69 lines).
patching file modules/system/system.install
Hunk #1 FAILED at 344.
Hunk #2 succeeded at 2385 with fuzz 1 (offset -1608 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/system/system.install.rej
patching file modules/tracker/tracker.pages.inc
Hunk #1 succeeded at 36 (offset -12 lines).

And this needs to go into 7.x now.

subscribing

Bump. +1.

This is a bug, and not really an API change, leaving at 7.x for now.