Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40889

apply 304 minifier fix to js too

    Details

    • Testing Instructions:
      Hide

      1/ purge caches
      2/ make sure the $CFG->cachejs is enabled
      3/ verify javascript served via lib/jabascroipt.php and YUI and theme JS works

      Note: there is no need to test master because the minify integration was reworked in MDL-40995

      Show
      1/ purge caches 2/ make sure the $CFG->cachejs is enabled 3/ verify javascript served via lib/jabascroipt.php and YUI and theme JS works Note: there is no need to test master because the minify integration was reworked in MDL-40995
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w32_MDL-40889_m26_jsminify

      Description

      see MDL-40569, we should apply the same fix to js_minify()

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dobedobedoh Andrew Nicols added a comment -

              Ack - please don't integrate this!! See MDL-40789 for more info, but this could possibly break things because it interferes with headers and we call it in page code.

              I've found what I hope is a more complete alternative in MDL-40789.

              Also, this has not been peer reviewed.

              Show
              dobedobedoh Andrew Nicols added a comment - Ack - please don't integrate this!! See MDL-40789 for more info, but this could possibly break things because it interferes with headers and we call it in page code. I've found what I hope is a more complete alternative in MDL-40789 . Also, this has not been peer reviewed.
              Hide
              poltawski Dan Poltawski added a comment - - edited

              Reopening based on Andrew's comments.

              Show
              poltawski Dan Poltawski added a comment - - edited Reopening based on Andrew's comments.
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              skodak Petr Skoda added a comment -

              We are using these headers in very few places, I am wondering what caching breakage are you talking about. Anyway I will look at your patch again and the minify, last time it seemed to me your patch would not work (may be it was just CSS).

              Show
              skodak Petr Skoda added a comment - We are using these headers in very few places, I am wondering what caching breakage are you talking about. Anyway I will look at your patch again and the minify, last time it seemed to me your patch would not work (may be it was just CSS).
              Hide
              skodak Petr Skoda added a comment -

              Looks like patch from Andrew should work better, but it needs the ststus code verification to prevent future problems.

              Show
              skodak Petr Skoda added a comment - Looks like patch from Andrew should work better, but it needs the ststus code verification to prevent future problems.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Petr,

              I only said that it could possibly break things

              But even if it doesn't now, it could cause issues down the road, and it doesn't seem like a good idea to be unsetting or modifying headers in our JS libraries, when they could be (and are) included in other code which is used for standard page generation.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Petr, I only said that it could possibly break things But even if it doesn't now, it could cause issues down the road, and it doesn't seem like a good idea to be unsetting or modifying headers in our JS libraries, when they could be (and are) included in other code which is used for standard page generation.
              Hide
              skodak Petr Skoda added a comment -

              Resubmitting for integration because the patch proposed in MDL-40789 did not work for some unknown reason for me, stable branches use the js minification only in scripts that are not affected by the unsetting of caching properties in _SERVER global.

              MDL-40995 contains a fully rewritten minify support that should be fixing all known and potential issues. I have discussed this with Andrew already, he is going to review it.

              Show
              skodak Petr Skoda added a comment - Resubmitting for integration because the patch proposed in MDL-40789 did not work for some unknown reason for me, stable branches use the js minification only in scripts that are not affected by the unsetting of caching properties in _SERVER global. MDL-40995 contains a fully rewritten minify support that should be fixing all known and potential issues. I have discussed this with Andrew already, he is going to review it.
              Hide
              marina Marina Glancy added a comment -

              I can two similar issues, this and MDL-40789. Andrew Nicols do you agree with this approach?
              Petr, since MDL-40995 is already integrated, is unsetting $_SERVER vars is still needed in master?

              Show
              marina Marina Glancy added a comment - I can two similar issues, this and MDL-40789 . Andrew Nicols do you agree with this approach? Petr, since MDL-40995 is already integrated, is unsetting $_SERVER vars is still needed in master?
              Hide
              skodak Petr Skoda added a comment -

              MDL-40995 integration means that we only need to apply patches for stable branches, it is already in master.

              Show
              skodak Petr Skoda added a comment - MDL-40995 integration means that we only need to apply patches for stable branches, it is already in master.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Integrated thanks Petr.

              Show
              samhemelryk Sam Hemelryk added a comment - Integrated thanks Petr.
              Hide
              ankit_frenz Ankit Agarwal added a comment -

              I guess the testing instructions meant to say lib/javascript.php and not lib/jabascroipt.php?
              If so, the issue passes.
              I tested a few things randomly, nothing seems to be broken.
              Thanks

              Show
              ankit_frenz Ankit Agarwal added a comment - I guess the testing instructions meant to say lib/javascript.php and not lib/jabascroipt.php? If so, the issue passes. I tested a few things randomly, nothing seems to be broken. Thanks
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better!

              "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar."
              ~ Professor Farnsworth

              Show
              samhemelryk Sam Hemelryk added a comment - Huzzah, your code made it into Moodle. Perhaps now things are ever so slightly better! "The ship can't take this much pressure. Sometimes it falls apart just sitting in the hangar." ~ Professor Farnsworth

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    9/Sep/13