Moodle

$CFG->filelifetime = 0 has no effect

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.7, 1.7.1, 1.7.2, 1.8, 1.8.1, 1.8.2
  • Fix Version/s: 1.8.3, 1.9
  • Component/s: Resource
  • Labels:
    None
  • Environment:
    Any
  • Affected Branches:
    MOODLE_17_STABLE, MOODLE_18_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE, MOODLE_19_STABLE

Description

Setting $CFG->filelifetime = 0 in config.php has no effect. This is because file.php checks to see if $CFG->filelifetime is empty. In PHP, empty(0) is true, which means your filelifetime is set to 86400, the default.

The workaround is to set $CFG->filelifetime = 1, but I think file.php should check to see if (!isset($CFG>filelifetime)) as per the attached patch. The value would be consistently applied throughout its possible range.

Attachments

  1. Text File
    file.php.patch
    25/Sep/07 5:26 PM
    0.6 kB
    Chris Fryer
  2. Text File
    filelib.php.patch
    26/Sep/07 8:22 PM
    1 kB
    Chris Fryer

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

thanks for the report and patch

Show
Petr Škoda (skodak) added a comment - thanks for the report and patch
Hide
Permalink
Chris Fryer added a comment -

Thanks. Will this be back-ported to MOODLE_18_STABLE?

Show
Chris Fryer added a comment - Thanks. Will this be back-ported to MOODLE_18_STABLE?
Hide
Permalink
Petr Škoda (skodak) added a comment -

Setting lifetime to 0 might cause server overloads. What is wrong with lifetime 60s for example?
Why do you want to use 0?

Show
Petr Škoda (skodak) added a comment - Setting lifetime to 0 might cause server overloads. What is wrong with lifetime 60s for example? Why do you want to use 0?
Hide
Permalink
Iñaki Arenaza added a comment -

Why not? Somethings you are really interested in setting it to 0 (I just needed to do it today, to be able to debug some proxy cache related issues where I needed to make sure the proxy wasn't caching the files locally).

Being able to set it to 0, and setting it to 0 are two different things. You want to give people enough rope, just in case they really want to hang themselves. Some will, and some will use the extra rope to do interesting (and non-harmful) things

Saludos. Iñaki.

Show
Iñaki Arenaza added a comment - Why not? Somethings you are really interested in setting it to 0 (I just needed to do it today, to be able to debug some proxy cache related issues where I needed to make sure the proxy wasn't caching the files locally). Being able to set it to 0, and setting it to 0 are two different things. You want to give people enough rope, just in case they really want to hang themselves. Some will, and some will use the extra rope to do interesting (and non-harmful) things Saludos. Iñaki.
Hide
Permalink
Petr Škoda (skodak) added a comment -

backported into MOODLE_18_STABLE too, I agree the debugging is a good enough reason

thanks for enlightening me!

Show
Petr Škoda (skodak) added a comment - backported into MOODLE_18_STABLE too, I agree the debugging is a good enough reason thanks for enlightening me!
Hide
Permalink
Chris Fryer added a comment -

How might it cause server overloads? Because file.php and send_file() must be called each time a file is requested? Or is it because a value of zero disables byteserving in send_file()?

Setting an arbitrary lifetime for files causes confusion for our editors. Whenever they upload a new copy of a file, then request it to check that everything's gone OK, their changes are not reflected. Needing to hit CTRL+F5 every time they make a change is counter-intuitive when the resource is not already bing displayed in the browser.

I think what you're really after is a way to send a 304 Not Modified header if the file has not changed since the last visit. In this case, $CFG->filelifetime is superfluous.

Here's some additional code for filelib.php that implements this:

$gmtlastmodified = gmdate('D, d M Y H:i:s', $lastmodified) . ' GMT';
header('Last-Modified: '. $gmtlastmodified);

if (isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) {
if (strtotime($gmtlastmodified) <= strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE'])) { header('HTTP/1.0 304 Not Modified'); header('Cache-control: must-revalidate'); exit (0); }
}

See also the attached patch.

Show
Chris Fryer added a comment - How might it cause server overloads? Because file.php and send_file() must be called each time a file is requested? Or is it because a value of zero disables byteserving in send_file()? Setting an arbitrary lifetime for files causes confusion for our editors. Whenever they upload a new copy of a file, then request it to check that everything's gone OK, their changes are not reflected. Needing to hit CTRL+F5 every time they make a change is counter-intuitive when the resource is not already bing displayed in the browser. I think what you're really after is a way to send a 304 Not Modified header if the file has not changed since the last visit. In this case, $CFG->filelifetime is superfluous. Here's some additional code for filelib.php that implements this: $gmtlastmodified = gmdate('D, d M Y H:i:s', $lastmodified) . ' GMT'; header('Last-Modified: '. $gmtlastmodified); if (isset($_SERVER['HTTP_IF_MODIFIED_SINCE'])) { if (strtotime($gmtlastmodified) <= strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE'])) { header('HTTP/1.0 304 Not Modified'); header('Cache-control: must-revalidate'); exit (0); } } See also the attached patch.
Hide
Permalink
Chris Fryer added a comment -

Implements 304 Not Modified in response to an If-Modified-Since request header.

Show
Chris Fryer added a comment - Implements 304 Not Modified in response to an If-Modified-Since request header.
Hide
Permalink
Petr Škoda (skodak) added a comment -

sure, it would be nice to have proper 304 support

The performance problem is that current file php must load user session and do basic access control checks - this is not cheap, the 304 would not eliminate this, if we allow caching in browsers the files are not requested at all.

This problem is clearly visible when using scorm with hundreds of small files.

Show
Petr Škoda (skodak) added a comment - sure, it would be nice to have proper 304 support The performance problem is that current file php must load user session and do basic access control checks - this is not cheap, the 304 would not eliminate this, if we allow caching in browsers the files are not requested at all. This problem is clearly visible when using scorm with hundreds of small files.
Hide
Permalink
Patrick Pfeifer added a comment -

I'd like to bring the issue about If-Modified-Since/304 Support up again because i ran into trouble recently trying to download some courses with httrack (website copier).
I'd like to run httrack like this:

$ httrack -A100000 -s0 -%s -%v2 https://moodle.fhnw.ch/user/view.php?id=2593 -* +moodle.fhnw.ch/user/view.php?id=2593* +moodle.fhnw.ch/course/view.php?id=978 +moodle.fhnw.ch/course/view.php?id=850 +moodle.fhnw.ch/course/view.php?id=967 +moodle.fhnw.ch/course/view.php?id=984 +moodle.fhnw.ch/mod/resource/* +moodle.fhnw.ch/file.php/* +.jpg +.gif +*.png

then when tachers upload new files, i'd just run

$ httrack --update

which downloads (in theory) all changed and added content from the site. The problem is that httrack uses If-Modified-Since to find out if files have changed and filelib.php still doesnt handle this. So it sends the File again and httrack downloads it although the file has not changed.

It would be great if Chris Fryer's patch could be somehow pulled into the mainline moodle. I dont quiet understand Petr Skoda's argumentation against it. "[...] if we allow caching [...]": I think enabling a proper If-Modified-Since support wouldn't "allow caching" any more then is already allowed. You cannot enforce anyway, if you allow retrieval. And the normal response headers are not changed at all. If a file got updated, it will be downloaded as before. There is no reason the browser wouldn't request changed files any more!

So please make moodle support If-Modified-Since/304 to save bandwidth.

Greets

Patrick

Show
Patrick Pfeifer added a comment - I'd like to bring the issue about If-Modified-Since/304 Support up again because i ran into trouble recently trying to download some courses with httrack (website copier). I'd like to run httrack like this: $ httrack -A100000 -s0 -%s -%v2 https://moodle.fhnw.ch/user/view.php?id=2593 -* +moodle.fhnw.ch/user/view.php?id=2593* +moodle.fhnw.ch/course/view.php?id=978 +moodle.fhnw.ch/course/view.php?id=850 +moodle.fhnw.ch/course/view.php?id=967 +moodle.fhnw.ch/course/view.php?id=984 +moodle.fhnw.ch/mod/resource/* +moodle.fhnw.ch/file.php/* +.jpg +.gif +*.png then when tachers upload new files, i'd just run $ httrack --update which downloads (in theory) all changed and added content from the site. The problem is that httrack uses If-Modified-Since to find out if files have changed and filelib.php still doesnt handle this. So it sends the File again and httrack downloads it although the file has not changed. It would be great if Chris Fryer's patch could be somehow pulled into the mainline moodle. I dont quiet understand Petr Skoda's argumentation against it. "[...] if we allow caching [...]": I think enabling a proper If-Modified-Since support wouldn't "allow caching" any more then is already allowed. You cannot enforce anyway, if you allow retrieval. And the normal response headers are not changed at all. If a file got updated, it will be downloaded as before. There is no reason the browser wouldn't request changed files any more! So please make moodle support If-Modified-Since/304 to save bandwidth. Greets Patrick

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.5#665-sha1:422aead) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.