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

          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