Moodle
  1. Moodle
  2. MDL-32683

optimise static file caching - fix headers and use slash arguments

    Details

    • Testing Instructions:
      Hide

      We need to test that we are not breaking file servering anywhere and that we are not ending up with broken links/resources, so there there are several parameters that affect static file serving and caching which will be need to be tried disabeld and enabled:

      1/ $CFG->slasharguments
      2/ $CFG->cachejs
      3/ $CFG->themedesignermode
      4/ $CFG->yuicomboloading
      5/ ssl mode
      6/ server type (IIS, Apache)
      7/ proxy servers - cloudfare, squid, etc.

      In order to test this we need to test all permutations in all supported browsers and for all themes.

      I know it is going to be a pain, but the perf benefits of this change could be huge on some sites.

      What to look for:

      • notices in PHP logs
      • missing files in apache log
      • errors in JS console
      • missing images
      • non-functional JS
      Show
      We need to test that we are not breaking file servering anywhere and that we are not ending up with broken links/resources, so there there are several parameters that affect static file serving and caching which will be need to be tried disabeld and enabled: 1/ $CFG->slasharguments 2/ $CFG->cachejs 3/ $CFG->themedesignermode 4/ $CFG->yuicomboloading 5/ ssl mode 6/ server type (IIS, Apache) 7/ proxy servers - cloudfare, squid, etc. In order to test this we need to test all permutations in all supported browsers and for all themes. I know it is going to be a pain, but the perf benefits of this change could be huge on some sites. What to look for: notices in PHP logs missing files in apache log errors in JS console missing images non-functional JS
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w19_MDL-32683_m23_publiccache
    • Rank:
      39634

      Description

      https://developers.google.com/speed/docs/best-practices/caching

      1/ it should help FF to cache SSL
      2/ it should help proxy servers along the way
      3/ more files should be served from browser caches

      we could use it for: themes, yui, javascript, etc.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          The patch might also resolve notices produced from the minifier and adds xsendfile support to lib/javascript.php.

          Show
          Petr Škoda added a comment - The patch might also resolve notices produced from the minifier and adds xsendfile support to lib/javascript.php.
          Hide
          Dan Poltawski added a comment -

          Hi Petr,

          Could you explain a bit more what you've done in this change, its not clear from the issue title/description

          Show
          Dan Poltawski added a comment - Hi Petr, Could you explain a bit more what you've done in this change, its not clear from the issue title/description
          Hide
          Petr Škoda added a comment - - edited

          The changes are described in https://developers.google.com/speed/docs/best-practices/caching:
          1/ it adds missing etags and other caching headers (to encourage as much caching as possible both in browsers and proxy caches)
          2/ it uses slash arguments for static files without cookies (? in url often prevents proxy caching)
          3/ it standardises lib/javascript.php serving (xsendfile, compression, no weird notices from minify caching, etc.)

          Show
          Petr Škoda added a comment - - edited The changes are described in https://developers.google.com/speed/docs/best-practices/caching: 1/ it adds missing etags and other caching headers (to encourage as much caching as possible both in browsers and proxy caches) 2/ it uses slash arguments for static files without cookies (? in url often prevents proxy caching) 3/ it standardises lib/javascript.php serving (xsendfile, compression, no weird notices from minify caching, etc.)
          Hide
          Dan Poltawski added a comment -

          Hmm. interesting. I'm seeing a weird CSS issue after adding this. See screenshot.

          Show
          Dan Poltawski added a comment - Hmm. interesting. I'm seeing a weird CSS issue after adding this. See screenshot.
          Hide
          Petr Škoda added a comment -

          oh, I forgot to mention you need to purge caches (it will be done automatically in upgrade), also we will have to force upgrade from main /index.php (but it is required for other changes introduced recently elsewhere). Please note you need to reset again if you use the same dataroot for checkouts without this patch.

          Show
          Petr Škoda added a comment - oh, I forgot to mention you need to purge caches (it will be done automatically in upgrade), also we will have to force upgrade from main /index.php (but it is required for other changes introduced recently elsewhere). Please note you need to reset again if you use the same dataroot for checkouts without this patch.
          Hide
          Petr Škoda added a comment -

          hmm, it might be better to bump up main version.php and force redirect_if_major_upgrade_required() right now and if necessary do it once again right before the release.

          Show
          Petr Škoda added a comment - hmm, it might be better to bump up main version.php and force redirect_if_major_upgrade_required() right now and if necessary do it once again right before the release.
          Hide
          Dan Poltawski added a comment -

          I have reset caches a few times, still seeing this weird theme thing.

          Show
          Dan Poltawski added a comment - I have reset caches a few times, still seeing this weird theme thing.
          Hide
          Petr Škoda added a comment -

          strange, what browser are you using?

          Show
          Petr Škoda added a comment - strange, what browser are you using?
          Hide
          Dan Poltawski added a comment -

          Chrome (I wonder if its some CSS which was previously not properly loaded!)

          Show
          Dan Poltawski added a comment - Chrome (I wonder if its some CSS which was previously not properly loaded!)
          Hide
          Petr Škoda added a comment -

          Chrome works fine here.

          Show
          Petr Škoda added a comment - Chrome works fine here.
          Hide
          Dan Poltawski added a comment - - edited

          Confirming this change seems to be unrelated to the theme issues I was seeing. In fact it was from using a standardold in the course theme!

          Show
          Dan Poltawski added a comment - - edited Confirming this change seems to be unrelated to the theme issues I was seeing. In fact it was from using a standardold in the course theme!
          Hide
          Dan Poltawski added a comment -

          Just a note that the caching header mentioned is: 'cache-control: public'

          Show
          Dan Poltawski added a comment - Just a note that the caching header mentioned is: 'cache-control: public'
          Hide
          Dan Poltawski added a comment -

          OK, i've spent quite a bit of time playing around with this and think its looking good so i've integrated this, thanks!

          Show
          Dan Poltawski added a comment - OK, i've spent quite a bit of time playing around with this and think its looking good so i've integrated this, thanks!
          Hide
          Dan Poltawski added a comment -

          Grr!
          user_picture_testcase::test_get_url
          Failed asserting that two strings are equal.
          — Expected
          +++ Actual
          @@ @@
          -'http://www.example.com/moodle/theme/image.php?theme=standard&image=u%2Ff2&rev=1'
          +'http://www.example.com/moodle/theme/image.php/standard/core/1/u/f2'

          /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/outputcomponents_test.php:174
          /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/lib.php:1411

          Show
          Dan Poltawski added a comment - Grr! user_picture_testcase::test_get_url Failed asserting that two strings are equal. — Expected +++ Actual @@ @@ -'http://www.example.com/moodle/theme/image.php?theme=standard&image=u%2Ff2&rev=1' +'http://www.example.com/moodle/theme/image.php/standard/core/1/u/f2' /Users/Shared/Jenkins/Home/git_repositories/master/lib/tests/outputcomponents_test.php:174 /Users/Shared/Jenkins/Home/git_repositories/master/lib/phpunit/lib.php:1411
          Hide
          Dan Poltawski added a comment -

          Breaks the unit tests!

          Show
          Dan Poltawski added a comment - Breaks the unit tests!
          Hide
          Petr Škoda added a comment -

          arrgh, I forgot that part, working on a fix right now

          Show
          Petr Škoda added a comment - arrgh, I forgot that part, working on a fix right now
          Hide
          Petr Škoda added a comment -

          fixes, sorry

          Show
          Petr Škoda added a comment - fixes, sorry
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks

          Show
          Dan Poltawski added a comment - Integrated, thanks
          Hide
          Petr Škoda added a comment -

          it would be good to test it together with MDL-32825

          Show
          Petr Škoda added a comment - it would be good to test it together with MDL-32825
          Hide
          Petr Škoda added a comment -

          I have discovered a regression in lib/javascript.php "js_send_uncached($content, $etag);", it is fixed in the linked issue MDL-32825

          Show
          Petr Škoda added a comment - I have discovered a regression in lib/javascript.php "js_send_uncached($content, $etag);", it is fixed in the linked issue MDL-32825
          Hide
          Andrew Nicols added a comment -

          This is causing JS breakages where the component isn't specified as an arg to M.util.image_url. See MDL-32745 for an example.
          Have patch - just testing it.

          Show
          Andrew Nicols added a comment - This is causing JS breakages where the component isn't specified as an arg to M.util.image_url. See MDL-32745 for an example. Have patch - just testing it.
          Hide
          Petr Škoda added a comment -

          oh! thanks for spotting it

          Show
          Petr Škoda added a comment - oh! thanks for spotting it
          Hide
          Dan Poltawski added a comment -

          Going to fail this test for the time being.

          Show
          Dan Poltawski added a comment - Going to fail this test for the time being.
          Hide
          Andrew Nicols added a comment -

          This should set a default component of 'core' now:

          git://git.luns.net.uk/
          MDL-32683-master-1
          https://git.luns.net.uk/moodle.git/commitdiff/b003b556cc7af38766de1d76fcff6f0f9d31bc73

          Show
          Andrew Nicols added a comment - This should set a default component of 'core' now: git://git.luns.net.uk/ MDL-32683 -master-1 https://git.luns.net.uk/moodle.git/commitdiff/b003b556cc7af38766de1d76fcff6f0f9d31bc73
          Hide
          Petr Škoda added a comment -

          thanks, looks ok! I am not sure which language I hate more, JS or PHP?

          Show
          Petr Škoda added a comment - thanks, looks ok! I am not sure which language I hate more, JS or PHP?
          Hide
          Petr Škoda added a comment - - edited

          sorry for the trouble, I hope any potential perf improvement will be worth all this.

          Show
          Petr Škoda added a comment - - edited sorry for the trouble, I hope any potential perf improvement will be worth all this.
          Hide
          Dan Poltawski added a comment -

          Pushed that, thanks Andrew!

          Show
          Dan Poltawski added a comment - Pushed that, thanks Andrew!
          Hide
          Petr Škoda added a comment -
          Show
          Petr Škoda added a comment - one more sloppy regression - please merge https://github.com/skodak/moodle/commit/1c6d3af542b2945eac424b80da68efe359fc3d0b
          Show
          Petr Škoda added a comment - and one more tweak https://github.com/skodak/moodle/commit/acc165aac781f5e51032dd10602d565bd50908b0
          Hide
          Petr Škoda added a comment -

          both added to my original PULL request branch

          Show
          Petr Škoda added a comment - both added to my original PULL request branch
          Hide
          Dan Poltawski added a comment -

          Pushed these fixes. Thanks

          Show
          Dan Poltawski added a comment - Pushed these fixes. Thanks
          Hide
          Petr Škoda added a comment -

          I have created a new issue MDL-32849 for sites that use URL rewriting, because this patch might create problems for them (they would have to modify some of the old recommended regex rules).

          Show
          Petr Škoda added a comment - I have created a new issue MDL-32849 for sites that use URL rewriting, because this patch might create problems for them (they would have to modify some of the old recommended regex rules).
          Hide
          Adrian Greeve added a comment -

          I tested this using IE, Opera, Firefox and Chrome. It took a lot of time going through the different settings with the different themes in the different browsers. I'm not exactly sure how complete my testing was but some things that I did do were:

          • Add each of the different resources
          • Add each of the different activities
          • Change over an old assignment to a new one
          • upload images from my computer and repositories
          • Load up data presets
          • Upload a csv file
          • Enabled ajax and moved course elements around
          • created chat messages in the chat window
          • Editing settings and grades in the grade book
          • imported quiz questions into the question bank

          I didn't come across any missing images or non-functional JS.
          I got a lot of strict rule notices and warnings when ever I was using the file picker and uploading data to the site.

          I got a couple of javascript error messages while upgrading all of the old assignments to the new assignment type.

          Message: 'M.str.assign.noassignmentsselected' is null or not an object Line: 59

          Message: 'null' is null or not an object Line: 27

          both in admin/tool/assignmentupgrade/module.js

          Neither of these errors actually effect how the upgrade works. Everything still gets converted ok.

          I hope this helps.

          Show
          Adrian Greeve added a comment - I tested this using IE, Opera, Firefox and Chrome. It took a lot of time going through the different settings with the different themes in the different browsers. I'm not exactly sure how complete my testing was but some things that I did do were: Add each of the different resources Add each of the different activities Change over an old assignment to a new one upload images from my computer and repositories Load up data presets Upload a csv file Enabled ajax and moved course elements around created chat messages in the chat window Editing settings and grades in the grade book imported quiz questions into the question bank I didn't come across any missing images or non-functional JS. I got a lot of strict rule notices and warnings when ever I was using the file picker and uploading data to the site. I got a couple of javascript error messages while upgrading all of the old assignments to the new assignment type. Message: 'M.str.assign.noassignmentsselected' is null or not an object Line: 59 Message: 'null' is null or not an object Line: 27 both in admin/tool/assignmentupgrade/module.js Neither of these errors actually effect how the upgrade works. Everything still gets converted ok. I hope this helps.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: