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

      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.

        Gliffy Diagrams

          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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: