Moodle
  1. Moodle
  2. MDL-35230

Empty Header and Footer Javascript links still written when not needed

    Details

    • Testing Instructions:
      Hide
      • Open your developer tools
      • Open the Network graph
      • Limit the scope to include just the Scripts

      Before applying the patch:

      • Refresh a Moodle page
      • Confirm that you see calls against javascript.php twice - once for type=head and once for type=footer

      After applying the patch

      • Refresh the Moodle page
      • Confirm that there are no entries for javascript.php

      Check that they do appear when required

      Note: This assumes you're using the Standard theme

      Now confirm that they do appear when they're meant to. We currently don't have any use of this in core so you'll have to hack some themes.

      • Edit /theme/base/config.php
      • Add:
        $THEME->javascripts = array(
          '35230',
        );
        
      • create a file in /theme/base/javascript/35230.js and give it some commented out content:
        // This is just a test of MDL-35230 in the header of the base theme
        
      • Refresh your page
      • Confirm that there is now an entry for javascript.php called against type=head and that the contents matches the file you just created
      • Confirm that there is only one entry for javascript.php
      • Edit /theme/standard/config.php
      • Add:
        $THEME->javascripts_footer = array(
          '35230',
        );
        
      • create a file in /theme/standard/javascript/35230.js and give it some commented out content:
        // This is just a test of MDL-35230 in the footer of the standard theme
        
      • Refresh your page
      • Confirm that there is an entry for javascript.php called against type=head and that the contents matches the file you created before
      • Confirm that there is an entry for javascript.php called against type=footer and that the contents matches the file you just created
      • Confirm that there are now two entries for javascript.php
      • Remove /theme/base/javascript/35230.js
      • Refresh the page
      • Confirm that there is an entry for javascript.php called against type=footer and that the contents matches the file you just created
      • Confirm that there are now only one entry for javascript.php (type=head has gone)
      • Edit /theme/standard/config.php
      • Remove the $THEME->javascripts_footer code
      • Refresh the page
      • Confirm that there are now no entries for
      Show
      Open your developer tools Open the Network graph Limit the scope to include just the Scripts Before applying the patch: Refresh a Moodle page Confirm that you see calls against javascript.php twice - once for type=head and once for type=footer After applying the patch Refresh the Moodle page Confirm that there are no entries for javascript.php Check that they do appear when required Note: This assumes you're using the Standard theme Now confirm that they do appear when they're meant to. We currently don't have any use of this in core so you'll have to hack some themes. Edit /theme/base/config.php Add: $THEME->javascripts = array( '35230', ); create a file in /theme/base/javascript/35230.js and give it some commented out content: // This is just a test of MDL-35230 in the header of the base theme Refresh your page Confirm that there is now an entry for javascript.php called against type=head and that the contents matches the file you just created Confirm that there is only one entry for javascript.php Edit /theme/standard/config.php Add: $THEME->javascripts_footer = array( '35230', ); create a file in /theme/standard/javascript/35230.js and give it some commented out content: // This is just a test of MDL-35230 in the footer of the standard theme Refresh your page Confirm that there is an entry for javascript.php called against type=head and that the contents matches the file you created before Confirm that there is an entry for javascript.php called against type=footer and that the contents matches the file you just created Confirm that there are now two entries for javascript.php Remove /theme/base/javascript/35230.js Refresh the page Confirm that there is an entry for javascript.php called against type=footer and that the contents matches the file you just created Confirm that there are now only one entry for javascript.php (type=head has gone) Edit /theme/standard/config.php Remove the $THEME->javascripts_footer code Refresh the page Confirm that there are now no entries for
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-35230-master
    • Rank:
      43878

      Description

      I don't have any theme javascript but I still get two links to javascript that look like this:

      [[<script type="text/javascript" src="http://moodle2.gla.ac.uk/theme/javascript.php/gu23/1346316913/footer"></script>]]

      The one at the start points to http://moodle2.gla.ac.uk/theme/javascript.php/gu23/1346316913/head

      Since these are both empty it would be good if Moodle was aware of that and didn't output the links.

        Activity

        Hide
        Andrew Nicols added a comment -

        This is probably a master-only change but it should be safe to apply the patch to a 2.3 site.

        Show
        Andrew Nicols added a comment - This is probably a master-only change but it should be safe to apply the patch to a 2.3 site.
        Hide
        Andrew Nicols added a comment -

        Adding Mary Evans as a watcher so she's aware of the theme-related change.

        Show
        Andrew Nicols added a comment - Adding Mary Evans as a watcher so she's aware of the theme-related change.
        Hide
        Mary Evans added a comment -

        @Andrew

        Base theme carries the following empty settings...

        $THEME->javascripts = array();
        $THEME->javascripts_footer = array();
        

        If these where commented out then the links would not show. True?

        If this is the case, then also commenting out these empty settings in Base theme would help too.

        Show
        Mary Evans added a comment - @Andrew Base theme carries the following empty settings... $THEME->javascripts = array(); $THEME->javascripts_footer = array(); If these where commented out then the links would not show. True? If this is the case, then also commenting out these empty settings in Base theme would help too.
        Hide
        Andrew Nicols added a comment -

        They wouldn't need to be commented out actually - the patch works by checking whether any of the themes in the parent theme path + the specific theme have anything in these arrays. If they do, then they're included.

        It shouldn't make any different to any core themes, and I doubt that anyone is actually using it outside of core much either.

        Show
        Andrew Nicols added a comment - They wouldn't need to be commented out actually - the patch works by checking whether any of the themes in the parent theme path + the specific theme have anything in these arrays. If they do, then they're included. It shouldn't make any different to any core themes, and I doubt that anyone is actually using it outside of core much either.
        Hide
        Andrew Davis added a comment -

        [Y] Syntax
        [Y] Output
        [Y] Whitespace
        [NA] Language
        [NA] Databases
        [?] Testing
        [Y] Security
        [NA] Documentation
        [Y] Git
        [Y] Sanity check

        The only minor point is to correct the testing instructions. Remove the stuff about before the patch. By the time this gets to a tester their install will have the fix installed and we're well past the point of replicating the bug.

        Show
        Andrew Davis added a comment - [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [?] Testing [Y] Security [NA] Documentation [Y] Git [Y] Sanity check The only minor point is to correct the testing instructions. Remove the stuff about before the patch. By the time this gets to a tester their install will have the fix installed and we're well past the point of replicating the bug.
        Hide
        Andrew Nicols added a comment -

        Submitting for Integration. However, I'm going to politely decline on changing the testing instructions (Sorry Andrew). I think that it is sometimes valuable to verify the presence of something before removing it otherwise it may be difficult to accurately say that it's gone.
        I'll leave it up to the tester's discretion.

        Show
        Andrew Nicols added a comment - Submitting for Integration. However, I'm going to politely decline on changing the testing instructions (Sorry Andrew). I think that it is sometimes valuable to verify the presence of something before removing it otherwise it may be difficult to accurately say that it's gone. I'll leave it up to the tester's discretion.
        Hide
        Dan Poltawski added a comment -

        Thanks Andrew, i've integrated that now (master only).

        Show
        Dan Poltawski added a comment - Thanks Andrew, i've integrated that now (master only).
        Hide
        Jason Fowler added a comment -

        Thanks for the fix Andrew, works fine

        Show
        Jason Fowler added a comment - Thanks for the fix Andrew, works fine
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And your fantastic code has met core, hope they become good friends for a long period.

        Closing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: