Moodle
  1. Moodle
  2. MDL-31079

Non-standard/buggy theme javascript causes minify to fail completely

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Environment:
      LAMP
    • Testing Instructions:
      Hide

      For developers:
      1/ add some non-standard javascript to some theme (see description if you need example)
      2/ verify minimisation failure is logged in console and the file itself
      3/ create a page that requires faulty js script, verify console logged the problem

      no idea how to create faulty css, sorry

      Show
      For developers: 1/ add some non-standard javascript to some theme (see description if you need example) 2/ verify minimisation failure is logged in console and the file itself 3/ create a page that requires faulty js script, verify console logged the problem no idea how to create faulty css, sorry
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w10_MDL-31079_m23_minifydebug

      Description

      When adding a javascript file via a theme config, a line like (this is jquery):

      if (href.search(/^[^:/]:\/\/[^/]\/?/) == -1){

      }

      is executed by browser despite the fact that the regex is not properly quoted. This creates fatal problems when minifying the javascript in non-developer modes in Moodle. There are other types of incompatible JS constructs that may be silently ignored by some browsers.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Amy Groshek added a comment -

            Cloning this issue. I think it should be looked at again. Our standard for javascript should not be a minifier. Our standard should be the browser, and possibly jslint or something like that. We have JS developers wasting hours and days trying to figure out why what they develop in theme designer mode is suddenly broken. Most of them give up and load their scripts via script tags in the layout files. You have to make this easier to use if you're going to ask us to use it.

            The minifier is responsible for notifying the JS developer of a failure. At the very least, in browsers that support it, we should be given a console.log() to tell us what JS file is causing the minifier to fail, and preferably what line of the file it's failing on. In addition, I should be able to turn the minifier off altogether, and still use the cache (presumably I'm savvy enough to minify my own JS files before adding them to Moodle).

            Show
            Amy Groshek added a comment - Cloning this issue. I think it should be looked at again. Our standard for javascript should not be a minifier. Our standard should be the browser, and possibly jslint or something like that. We have JS developers wasting hours and days trying to figure out why what they develop in theme designer mode is suddenly broken. Most of them give up and load their scripts via script tags in the layout files. You have to make this easier to use if you're going to ask us to use it. The minifier is responsible for notifying the JS developer of a failure. At the very least, in browsers that support it, we should be given a console.log() to tell us what JS file is causing the minifier to fail, and preferably what line of the file it's failing on. In addition, I should be able to turn the minifier off altogether, and still use the cache (presumably I'm savvy enough to minify my own JS files before adding them to Moodle).
            Hide
            Petr Skoda added a comment -

            Hello, please submit the exact steps and code to reproduce this problem, please note that jquery is not supported at all. Thanks for the report anyway.

            Petr

            Show
            Petr Skoda added a comment - Hello, please submit the exact steps and code to reproduce this problem, please note that jquery is not supported at all. Thanks for the report anyway. Petr
            Hide
            Amy Groshek added a comment -
            Show
            Amy Groshek added a comment - Steps to reproduce are in http://tracker.moodle.org/browse/MDL-26315
            Hide
            Amy Groshek added a comment - - edited

            This is not about jquery. I pass jquery already minified via config.php. It does not need to be minified. The minifier is not passing adequate information to the developers. I should be able to turn the minifier off if I want to, and still use the cache. This is also about being able to pass scripts to a child theme. It's not just about jquery.

            If this isn't fixed it will seriously interfere with our ability to add javascript interactivity to customer content.

            Show
            Amy Groshek added a comment - - edited This is not about jquery. I pass jquery already minified via config.php. It does not need to be minified. The minifier is not passing adequate information to the developers. I should be able to turn the minifier off if I want to, and still use the cache. This is also about being able to pass scripts to a child theme. It's not just about jquery. If this isn't fixed it will seriously interfere with our ability to add javascript interactivity to customer content.
            Hide
            Petr Skoda added a comment -

            Ah, sorry, the cloned issue confused me, it would be easier if you just commented in the original issue. I am going to fix the title and description.

            Show
            Petr Skoda added a comment - Ah, sorry, the cloned issue confused me, it would be easier if you just commented in the original issue. I am going to fix the title and description.
            Hide
            Petr Skoda added a comment -

            I agree we could at least try to write something to the console log when the compression fails for some reason, any idea how to do that?

            Show
            Petr Skoda added a comment - I agree we could at least try to write something to the console log when the compression fails for some reason, any idea how to do that?
            Hide
            Amy Groshek added a comment - - edited

            Petr, thank you for your attention to this. We appreciate your time.

            My hope would be that the minify script would provide some sort of error reporting and we could write a javascript warning to the page or load a javascript file to the page which would write that warning. I see that minify.php does return a 500 error when jsmin chokes on a file: http://code.google.com/p/minify/wiki/Debugging. We could check for a 500 error and then call a JS script to log the warning? I'm not that great with PHP so I have no idea how this would be done.

            At the very least, the javascript would look something like this:

            console.warn('Moodle's minifier threw an exception on a javascript file being loaded by theme [themename]. Check the file for syntax errors.');
            

            If we send each file to the minifier before concatenation, this is better, because we can tell the theme designer which of her js files have caused the error.

            console.warn('Moodle's minifier threw an exception at [javascript file] in theme [themename]. Check the file for syntax errors.');
            

            I also see that minify Minify 2.1.4+ excludes files with 'min' in the file name from minification. Moodle is using 2.1.3. I have eliminated the problem twice by removing all minified libraries from my theme. So upgrading minify might resolve a great deal of these issues.

            I will do the following:
            1) Put together a theme which demonstrates this issue for testing
            2) Replace minify 2.1.3 with 2.1.4 on my local machine and see whether that resolves the issue with minified files

            I still think it would be good to implement a console.warn() in the event of a syntax-based error, rather than minification-based error. I will not be as helpful in this regard, but I can try.

            Show
            Amy Groshek added a comment - - edited Petr, thank you for your attention to this. We appreciate your time. My hope would be that the minify script would provide some sort of error reporting and we could write a javascript warning to the page or load a javascript file to the page which would write that warning. I see that minify.php does return a 500 error when jsmin chokes on a file: http://code.google.com/p/minify/wiki/Debugging . We could check for a 500 error and then call a JS script to log the warning? I'm not that great with PHP so I have no idea how this would be done. At the very least, the javascript would look something like this: console.warn( 'Moodle' s minifier threw an exception on a javascript file being loaded by theme [themename]. Check the file for syntax errors.'); If we send each file to the minifier before concatenation, this is better, because we can tell the theme designer which of her js files have caused the error. console.warn( 'Moodle' s minifier threw an exception at [javascript file] in theme [themename]. Check the file for syntax errors.'); I also see that minify Minify 2.1.4+ excludes files with 'min' in the file name from minification. Moodle is using 2.1.3. I have eliminated the problem twice by removing all minified libraries from my theme. So upgrading minify might resolve a great deal of these issues. I will do the following: 1) Put together a theme which demonstrates this issue for testing 2) Replace minify 2.1.3 with 2.1.4 on my local machine and see whether that resolves the issue with minified files I still think it would be good to implement a console.warn() in the event of a syntax-based error, rather than minification-based error. I will not be as helpful in this regard, but I can try.
            Hide
            Amy Groshek added a comment - - edited

            Theme which demonstrates this issue is here: git@github.com:amygroshek/MDL-31079-theme.git

            With theme designer mode on, console receives the following messages, indicating that all js files are loaded and library objects exist:
            mods.js loaded
            javascript.php:15jQuery exists. logging object.
            javascript.php:15function (a,b)

            {return new e.fn.init(a,b,h)}

            javascript.php:16Your browser supports SVG.
            You are running Raphaël 2.0.1

            With theme designer mode off, the following error is logged:
            Uncaught SyntaxError: Unexpected token .

            Also attaching PDF showing that my js file, mods.js, passes jslint.

            Show
            Amy Groshek added a comment - - edited Theme which demonstrates this issue is here: git@github.com:amygroshek/ MDL-31079 -theme.git With theme designer mode on, console receives the following messages, indicating that all js files are loaded and library objects exist: mods.js loaded javascript.php:15jQuery exists. logging object. javascript.php:15function (a,b) {return new e.fn.init(a,b,h)} javascript.php:16Your browser supports SVG. You are running Raphaël 2.0.1 With theme designer mode off, the following error is logged: Uncaught SyntaxError: Unexpected token . Also attaching PDF showing that my js file, mods.js, passes jslint.
            Hide
            Amy Groshek added a comment -

            I replaced the minify 1.3+ library with the minify 1.4+ library, tested the above theme, and experienced the same results. 2.1.4, according to Minify documentation, should not touch files with .min or -min in the filename. It's possible that I did not replace the lib correctly. However, it's also possible that the minify/caching process is failing elsewhere.

            Show
            Amy Groshek added a comment - I replaced the minify 1.3+ library with the minify 1.4+ library, tested the above theme, and experienced the same results. 2.1.4, according to Minify documentation, should not touch files with .min or -min in the filename. It's possible that I did not replace the lib correctly. However, it's also possible that the minify/caching process is failing elsewhere.
            Hide
            Petr Skoda added a comment -

            thanks for the report and cooperation

            Show
            Petr Skoda added a comment - thanks for the report and cooperation
            Hide
            Petr Skoda added a comment -

            Amy: could you please test my patch? you can use git to checkout my branch at github or you could download branch package

            Show
            Petr Skoda added a comment - Amy: could you please test my patch? you can use git to checkout my branch at github or you could download branch package
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm going to integrate this but, silly question... why are we duplicating so many code. Wouldn't be better to "unify" the "minify" (as much as possible, at least the executions and error handling). Feel free to create one new issue for that.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I'm going to integrate this but, silly question... why are we duplicating so many code. Wouldn't be better to "unify" the "minify" (as much as possible, at least the executions and error handling). Feel free to create one new issue for that. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Ops, sorry, rejecting. This is conflicting badly with the big change performed @ MDL-29941.

            So better delayed one week (consider if there are possibilities to centralize the execution).

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Ops, sorry, rejecting. This is conflicting badly with the big change performed @ MDL-29941 . So better delayed one week (consider if there are possibilities to centralize the execution). Ciao
            Hide
            Amy Groshek added a comment - - edited

            Hi Petr,

            Sorry for the delay on this. I just tested your branch. I'm using the following theme to test: git@github.com:amygroshek/MDL31079.git . I'm loving the debug messages you wrote into styles.php and javascript.php, but not seeing them in Chrome or FF when I test.

            Have cloned git://github.com/skodak/moodle.git and checked out w04_MDL-31079_m23_minifydebug . Also manually deleting cached themes from moodledata/cache/theme before each refresh. Not sure what I'm doing wrong.

            Error log shows the following. Clearly not changes from this branch... might be interfering with my testing? [Fri Feb 10 15:45:00 2012] [error] [client 127.0.0.1] PHP Warning: fclose() expects parameter 1 to be resource, boolean given in / Library/WebServer/Documents/minifydebug/moodle/theme/image.php on line 112

            Show
            Amy Groshek added a comment - - edited Hi Petr, Sorry for the delay on this. I just tested your branch. I'm using the following theme to test: git@github.com:amygroshek/MDL31079.git . I'm loving the debug messages you wrote into styles.php and javascript.php, but not seeing them in Chrome or FF when I test. Have cloned git://github.com/skodak/moodle.git and checked out w04_ MDL-31079 _m23_minifydebug . Also manually deleting cached themes from moodledata/cache/theme before each refresh. Not sure what I'm doing wrong. Error log shows the following. Clearly not changes from this branch... might be interfering with my testing? [Fri Feb 10 15:45:00 2012] [error] [client 127.0.0.1] PHP Warning: fclose() expects parameter 1 to be resource, boolean given in / Library/WebServer/Documents/minifydebug/moodle/theme/image.php on line 112
            Hide
            Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Petr Skoda added a comment -

            rebased

            Show
            Petr Skoda added a comment - rebased
            Hide
            Eloy Lafuente (stronk7) added a comment -

            As commented above... no chance to reduce the 3 usages of minify() to a common one, or at least the "output error" section ? It really looks doable.

            Show
            Eloy Lafuente (stronk7) added a comment - As commented above... no chance to reduce the 3 usages of minify() to a common one, or at least the "output error" section ? It really looks doable.
            Hide
            Petr Skoda added a comment -

            It would require bigger changes I am afraid - in any case one is for javascript, second for CSS. The other Javascript is using different caching code...

            Show
            Petr Skoda added a comment - It would require bigger changes I am afraid - in any case one is for javascript, second for CSS. The other Javascript is using different caching code...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (master only) thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (master only) thanks!
            Hide
            Rossiani Wijaya added a comment -

            This looks good.

            Test passed.

            Show
            Rossiani Wijaya added a comment - This looks good. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).

            icao_reverse('arreis olik rebemevon afla letoh ognat');
            

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!). icao_reverse('arreis olik rebemevon afla letoh ognat'); Closing, ciao

              People

              • Votes:
                13 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: