Details

    • Rank:
      51790

      Description

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

        Issue Links

          Activity

          Hide
          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
          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
          Dan Poltawski added a comment - - edited

          Reopening based on Andrew's comments.

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

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - Looks like patch from Andrew should work better, but it needs the ststus code verification to prevent future problems.
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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 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 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
          Petr Škoda added a comment -

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

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

          Integrated thanks Petr.

          Show
          Sam Hemelryk added a comment - Integrated thanks Petr.
          Hide
          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 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
          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
          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: