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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

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

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda added a comment -

            strange, what browser are you using?

            Show
            Petr Skoda 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 Skoda added a comment -

            Chrome works fine here.

            Show
            Petr Skoda 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 Skoda added a comment -

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

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

            fixes, sorry

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

            Integrated, thanks

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

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

            Show
            Petr Skoda added a comment - it would be good to test it together with MDL-32825
            Hide
            Petr Skoda 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 Skoda 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 Skoda added a comment -

            oh! thanks for spotting it

            Show
            Petr Skoda 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 Skoda added a comment -

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

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

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

            Show
            Petr Skoda 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 Skoda added a comment -
            Show
            Petr Skoda added a comment - one more sloppy regression - please merge https://github.com/skodak/moodle/commit/1c6d3af542b2945eac424b80da68efe359fc3d0b
            Show
            Petr Skoda added a comment - and one more tweak https://github.com/skodak/moodle/commit/acc165aac781f5e51032dd10602d565bd50908b0
            Hide
            Petr Skoda added a comment -

            both added to my original PULL request branch

            Show
            Petr Skoda 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 Skoda 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 Skoda 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: