Details

    • Testing Instructions:
      Hide

      1/ run phpunit tests
      2/ enable js cache and disable theme creator mode
      3/ purge caches and verify the js and css served to browser (it should be minified the same way as before)
      4/ delete $CFG->dataroot/localcache and reload page, verify the JS and CSS cache files get recreated properly
      5/ test TinyMCE works
      6/ verify the YUI_config module stuff embedded in pages is minified

      Show
      1/ run phpunit tests 2/ enable js cache and disable theme creator mode 3/ purge caches and verify the js and css served to browser (it should be minified the same way as before) 4/ delete $CFG->dataroot/localcache and reload page, verify the JS and CSS cache files get recreated properly 5/ test TinyMCE works 6/ verify the YUI_config module stuff embedded in pages is minified
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w32_MDL-40995_m26_minifyreworked
    • Rank:
      51896

      Description

      There are some new problems when minifying CSS and JS:
      1/ js minify is now used from outputrequirementslib file for compression of YUI module info
      2/ the latest minify uses autoloader - it would slow down the rest of moodle
      3/ the unsetting of SERVER variable is a nasty hack that might potentially break some caching
      4/ minify is intended for direct CSS/JS file serving, but we need only the compression of files
      5/ in csslib we first minify and then replace our placeholders, this is wrong; minify works with files only but we do not want to create any temporary files there

      Proposed solution:

      • create new core_minify class
      • with methods ::css(), ::css_files(), ::js(), js_files()
      • use only as little as possible from minify
      • replace minify loader or unregister it after use

      Nota that this does not affect stable branches much, no need to backport.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Just some initial comments as I start my review:

          1. It looks like we've moved from Minify to JSMinPlus. AFAIK, Minify doesn't use JSMinPlus (and I cant' see anywhere else in Moodle where it's used). Is this likely to have an effect on the effect of the minification. Changing libraries is obviously a big change and are we sure that it's stable enough to handle all of our non-lintworthy code.
          2. Could you add some comments around the ob_start() in js() to make it clear that it's purpose is error handling.
          3. Since the JsMinPlus::minify() function just runs everything through the parser and catches output (and echos it), do you feel it's worth replicating that in the new js() function you've written so that we don't have to catch the error with ob_start() etc?

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

          Whitespace: This is probably a personal preference, but whitespace either side of concats makes things sooo much easier to read IMO. I know it's not part of the coding guidelines

          Documentation: The phpdoc for the css() function is wrong - it's the JS doc

          I think we could do with just checking that we're entirely happy with the change from minify to JSMinPlus too. The unit tests for JSMinPlus.php look pretty limited (https://code.google.com/p/minify/source/browse/min_unit_tests/test_JSMinPlus.php?r=a24e9a4c22699acb1e81b2e8ca84ba0675d20e9c) and I think we could do with making sure that our non-linting code is fine. In particular, things like javascript-static.js.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Just some initial comments as I start my review: It looks like we've moved from Minify to JSMinPlus. AFAIK, Minify doesn't use JSMinPlus (and I cant' see anywhere else in Moodle where it's used). Is this likely to have an effect on the effect of the minification. Changing libraries is obviously a big change and are we sure that it's stable enough to handle all of our non-lintworthy code. Could you add some comments around the ob_start() in js() to make it clear that it's purpose is error handling. Since the JsMinPlus::minify() function just runs everything through the parser and catches output (and echos it), do you feel it's worth replicating that in the new js() function you've written so that we don't have to catch the error with ob_start() etc? [Y] Syntax [?] Whitespace [Y] Output [-] Language [-] Databases [Y] Testing [Y] Security [N] Documentation [Y] Git [Y] Sanity check Whitespace: This is probably a personal preference, but whitespace either side of concats makes things sooo much easier to read IMO. I know it's not part of the coding guidelines Documentation: The phpdoc for the css() function is wrong - it's the JS doc I think we could do with just checking that we're entirely happy with the change from minify to JSMinPlus too. The unit tests for JSMinPlus.php look pretty limited ( https://code.google.com/p/minify/source/browse/min_unit_tests/test_JSMinPlus.php?r=a24e9a4c22699acb1e81b2e8ca84ba0675d20e9c ) and I think we could do with making sure that our non-linting code is fine. In particular, things like javascript-static.js. Cheers, Andrew
          Hide
          Petr Škoda added a comment -

          1. No! We moved so JSMinPlus in 2.5 already because of the nonsense license in JSMin.
          2. Ob_start deals with error output from the library, I did not want to hack that - adding comment. See

          	private function min($js, $filename)
          	{
          		try
          		{
          			$n = $this->parser->parse($js, $filename, 1);
          			return $this->parseTree($n);
          		}
          		catch(Exception $e)
          		{
          			echo $e->getMessage() . "\n";
          		}
          
          		return false;
          	}
          

          3. I do not understand.

          Show
          Petr Škoda added a comment - 1. No! We moved so JSMinPlus in 2.5 already because of the nonsense license in JSMin. 2. Ob_start deals with error output from the library, I did not want to hack that - adding comment. See private function min($js, $filename) { try { $n = $ this ->parser->parse($js, $filename, 1); return $ this ->parseTree($n); } catch (Exception $e) { echo $e->getMessage() . "\n" ; } return false ; } 3. I do not understand.
          Hide
          Petr Škoda added a comment -

          ob_start() comment added, phpdocs fixed, I like the whitespace the way it was

          thanks a lot for the review

          Show
          Petr Škoda added a comment - ob_start() comment added, phpdocs fixed, I like the whitespace the way it was thanks a lot for the review
          Hide
          Petr Škoda added a comment - - edited

          Submitting for integration because 2.5 is already using JSMinPlus. If we need to switch to any other compression library it should be now a lot easier.

          From 2.5 stable:

          // JSMin is not GNU GPL compatible, use the plus version instead.
          'minifiers' => array(Minify::TYPE_JS => array('JSMinPlus', 'minify')),
          
          Show
          Petr Škoda added a comment - - edited Submitting for integration because 2.5 is already using JSMinPlus. If we need to switch to any other compression library it should be now a lot easier. From 2.5 stable: // JSMin is not GNU GPL compatible, use the plus version instead. 'minifiers' => array(Minify::TYPE_JS => array('JSMinPlus', 'minify')),
          Hide
          Andrew Nicols added a comment -

          1. Apologies - I was git grepping for use of JSMinPlus in your branch rather than Master.

          2. Yup - I'd worked out it's purpose (was just asking you to add a comment).

          3. Ignore me, I was just thinking aloud.

          Show
          Andrew Nicols added a comment - 1. Apologies - I was git grepping for use of JSMinPlus in your branch rather than Master. 2. Yup - I'd worked out it's purpose (was just asking you to add a comment). 3. Ignore me, I was just thinking aloud.
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Petr - this has been integrated now.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          From what I could tell, the minification is working consistently. I compared results to 25 and the same files were being minified.

          Caching purges and deletion did not affect the results.

          The embedded YUI config content was minified.

          Show
          Michael de Raadt added a comment - Test result: Success! From what I could tell, the minification is working consistently. I compared results to 25 and the same files were being minified. Caching purges and deletion did not affect the results. The embedded YUI config content was minified.
          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:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: