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
    • Rank:
      37500

      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.

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda added a comment -

          thanks for the report and cooperation

          Show
          Petr Škoda added a comment - thanks for the report and cooperation
          Hide
          Petr Škoda 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 Škoda 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 Škoda added a comment -

          rebased

          Show
          Petr Škoda 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 Škoda 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 Škoda 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: