Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-30659

Serve non-minified YUI3 in developer debug mode

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1.3, 2.2
    • Fix Version/s: 2.1.4, 2.2.1
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1/ install firebug in FF
      2/ enable Net tab
      3/ review the YUI2 and 3 CSS+JS in normal and dev debug mode
      4/ edit the outputrequirements.php $this->yui3loader->filter = YUI_DEBUG;, test that JS console shows YUI debug and loads debug files

      Show
      1/ install firebug in FF 2/ enable Net tab 3/ review the YUI2 and 3 CSS+JS in normal and dev debug mode 4/ edit the outputrequirements.php $this->yui3loader->filter = YUI_DEBUG;, test that JS console shows YUI debug and loads debug files
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w51_MDL-30659_m23_yuidebug

      Description

      I thought we had already implemented this, except that JS served via theme/yui_combo.php was still coming through as minified, so we need to do more.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            hmm, should not this be done only when yui is not told to cache stuff? otherwise switching on/off the debug mode will not work when caching enabled, right?

            Show
            skodak Petr Skoda added a comment - hmm, should not this be done only when yui is not told to cache stuff? otherwise switching on/off the debug mode will not work when caching enabled, right?
            Hide
            timhunt Tim Hunt added a comment -

            Petr, that is exactly why I made you a watcher here! Thank you.

            Becuase of ABORT_AFTER_CONFIG, the un-minified YUI will only be sent if $CFG->debug = 38911 is set in config.php. Would that ever happen on a production server?

            If this happens on a developers' server, would it really matter?

            If you think it is better, I could add $cache = false; inside the new if statement. What do you think is best.

            Also, it would be really nice to avoid hard-coding 38911, but I can't think of a way to achieve that.

            Note that this change is useful. It let me work out http://yuilibrary.com/projects/yui3/ticket/2531561

            Show
            timhunt Tim Hunt added a comment - Petr, that is exactly why I made you a watcher here! Thank you. Becuase of ABORT_AFTER_CONFIG, the un-minified YUI will only be sent if $CFG->debug = 38911 is set in config.php. Would that ever happen on a production server? If this happens on a developers' server, would it really matter? If you think it is better, I could add $cache = false; inside the new if statement. What do you think is best. Also, it would be really nice to avoid hard-coding 38911, but I can't think of a way to achieve that. Note that this change is useful. It let me work out http://yuilibrary.com/projects/yui3/ticket/2531561
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi guys,

            I was just having a look at this now, I was sure that this was working in the past.
            A bit of research and indeed it was working in the past but due to changes in the YUI loader, or perhaps just in YUI it no longer does.
            The solution appears to be pretty simple:

            diff --git a/lib/outputrequirementslib.php b/lib/outputrequirementslib.php
            index 93c5cbe..2401862 100644
            --- a/lib/outputrequirementslib.php
            +++ b/lib/outputrequirementslib.php
            @@ -160,7 +160,7 @@ class page_requirements_manager {
                     $this->M_yui_loader->base         = $this->yui3loader->base;
                     $this->M_yui_loader->comboBase    = $this->yui3loader->comboBase;
                     $this->M_yui_loader->combine      = $this->yui3loader->combine;
            -        $this->M_yui_loader->filter       = ($this->yui3loader->filter == YUI_DEBUG) ? 'debug' : '';
            +        $this->M_yui_loader->filter       = $this->yui3loader->filter;
                     $this->M_yui_loader->insertBefore = 'firstthemesheet';
                     $this->M_yui_loader->modules      = array();
                     $this->M_yui_loader->groups       = array(

            Basically it looks like things have changed from requiring the filter to be defined as 'debug' to being defined as one of 'DEBUG', or 'RAW' for which we can use the YUI defines YUI_DEBUG, and YUI_RAW.
            In our case we have traditionally used raw and I have left it at that.

            Would be great if someone else could test this.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi guys, I was just having a look at this now, I was sure that this was working in the past. A bit of research and indeed it was working in the past but due to changes in the YUI loader, or perhaps just in YUI it no longer does. The solution appears to be pretty simple: diff --git a/lib/outputrequirementslib.php b/lib/outputrequirementslib.php index 93c5cbe..2401862 100644 --- a/lib/outputrequirementslib.php +++ b/lib/outputrequirementslib.php @@ -160,7 +160,7 @@ class page_requirements_manager { $this->M_yui_loader->base = $this->yui3loader->base; $this->M_yui_loader->comboBase = $this->yui3loader->comboBase; $this->M_yui_loader->combine = $this->yui3loader->combine; - $this->M_yui_loader->filter = ($this->yui3loader->filter == YUI_DEBUG) ? 'debug' : ''; + $this->M_yui_loader->filter = $this->yui3loader->filter; $this->M_yui_loader->insertBefore = 'firstthemesheet'; $this->M_yui_loader->modules = array(); $this->M_yui_loader->groups = array( Basically it looks like things have changed from requiring the filter to be defined as 'debug' to being defined as one of 'DEBUG', or 'RAW' for which we can use the YUI defines YUI_DEBUG, and YUI_RAW. In our case we have traditionally used raw and I have left it at that. Would be great if someone else could test this. Cheers Sam
            Hide
            timhunt Tim Hunt added a comment -

            Aha! Thanks I will look at this tomorrow.

            In the past, we used to serve either debug or min YUI, but that was a real pain, because mostly yui-debug fills the javascript console with crap that dose not tell you anything useful, but which hides the real error.

            So we changed it to yui plain or min, but that line of code you point out probably needed to be changed and was not.

            Show
            timhunt Tim Hunt added a comment - Aha! Thanks I will look at this tomorrow. In the past, we used to serve either debug or min YUI, but that was a real pain, because mostly yui-debug fills the javascript console with crap that dose not tell you anything useful, but which hides the real error. So we changed it to yui plain or min, but that line of code you point out probably needed to be changed and was not.
            Hide
            skodak Petr Skoda added a comment -

            Ah, the yui3 loader is not used because it did not support the latest yui3 - I had to include the necessary JS manually and I forgot to add exception for the non-minified stuff. I will fix it somehow soon, thanks!

            Show
            skodak Petr Skoda added a comment - Ah, the yui3 loader is not used because it did not support the latest yui3 - I had to include the necessary JS manually and I forgot to add exception for the non-minified stuff. I will fix it somehow soon, thanks!
            Hide
            skodak Petr Skoda added a comment -

            I am working on a bit different fix...

            Show
            skodak Petr Skoda added a comment - I am working on a bit different fix...
            Hide
            skodak Petr Skoda added a comment - - edited

            hmm, I should have read the comment from Sam properly, I came independently to similar solution + some extra filter fix and phpdocs cleanup, thanks!

            To integrators: please note 21 branch does not contain phpdocs fixes intentionally

            Show
            skodak Petr Skoda added a comment - - edited hmm, I should have read the comment from Sam properly, I came independently to similar solution + some extra filter fix and phpdocs cleanup, thanks! To integrators: please note 21 branch does not contain phpdocs fixes intentionally
            Hide
            timhunt Tim Hunt added a comment -

            Looks good to me. Thanks for doing a proper fix Petr. +1

            Show
            timhunt Tim Hunt added a comment - Looks good to me. Thanks for doing a proper fix Petr. +1
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Petr, I've integrated the fix now

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr, I've integrated the fix now Cheers Sam
            Hide
            gerry Gerard Caulfield added a comment - - edited

            Looks great.

            Side note: The following bulky scripts never appear to be minified, however this was not part of the test:
            /theme/yui_combo.php?moodle/288/block_navigation/navigation/navigation.js
            /theme/yui_combo.php?moodle/288/calendar/eventmanager/eventmanager.js

            For part 4 I assumed outputrequirements[lib].php was meant.

            Show
            gerry Gerard Caulfield added a comment - - edited Looks great. Side note: The following bulky scripts never appear to be minified, however this was not part of the test: /theme/yui_combo.php?moodle/288/block_navigation/navigation/navigation.js /theme/yui_combo.php?moodle/288/calendar/eventmanager/eventmanager.js For part 4 I assumed outputrequirements [lib] .php was meant.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

            Now... disconnect, relax and enjoy the next days, yay!

            Closing...ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jan/12