Moodle
  1. Moodle
  2. MDL-32279

Text caching leads to missing inclusion of JS/YUI required stuff

    Details

    • Testing Instructions:
      Hide

      Preparation:

      1/ Enable glossary filter @ admin->plugins->filters->manage. (set to content and headings)
      2/ Ensure text caching is enabled (at least 1 minute) @ admin->plugins->filters->common settings.
      3/ Create one glossary with a few entries.
      4/ Create one forum post containing some of the concepts created in 3/. Save it.
      5/ TEST: The concepts are shown highlighted and are links.
      6/ TEST: Mouse over shows: "activity name: concept".
      7/ TEST: Clicking the link shows one JS popup with the definition of the concept and the browser continues in the same page.

      8/ Refresh the page (within the window of time configured in 2/, so cached versions will be shown).
      9/ TEST: Clicking the concept link shows one JS popup with the definition of the concept and the browser continues in the same page.

      10/ Disable JS.
      11/ Refresh the page.
      12/ TEST: Clicking the concept sends the browser to mod/glossary/showentry.php URL, instead of the JS version. The definition of the concept is shown there.

      Show
      Preparation: 1/ Enable glossary filter @ admin->plugins->filters->manage. (set to content and headings ) 2/ Ensure text caching is enabled (at least 1 minute) @ admin->plugins->filters->common settings. 3/ Create one glossary with a few entries. 4/ Create one forum post containing some of the concepts created in 3/. Save it. 5/ TEST: The concepts are shown highlighted and are links. 6/ TEST: Mouse over shows: "activity name: concept". 7/ TEST: Clicking the link shows one JS popup with the definition of the concept and the browser continues in the same page. 8/ Refresh the page (within the window of time configured in 2/, so cached versions will be shown). 9/ TEST: Clicking the concept link shows one JS popup with the definition of the concept and the browser continues in the same page. 10/ Disable JS. 11/ Refresh the page. 12/ TEST: Clicking the concept sends the browser to mod/glossary/showentry.php URL, instead of the JS version. The definition of the concept is shown there.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Using Firefox or Chrome when a user clicks on a glossary link, to open the page in the same window, a pin-wheel (loading icon) appears in the middle of the browser and the page never loads. However, when you right click and choose "open in tab" or "open in new window" the glossary page opens just fine.

      For once, I was not able to replicate the issue when using Internet Explorer.

      This maybe be a browser delivery error with the current versions of Firefox and Chrome?

      1. Enable the Glossary Autolinking filter at Site admin > Plugins > Filters > Manage filters
      2. Create a Glossary
      3. Add a term
      4. Use Concept or Keyword in another resource (in our case a page)
      5. Click on the link to the Glossary while using Firefox or Chrome
      6. Perform the same action from the previous step while using Internet Explorer
      7. In Chrome and Firefox the Pin-Wheel loading icon appears in the center of the page and never loads. In IE the page loads.

        Gliffy Diagrams

        1. Error from Firebug.txt
          1 kB
          Joseph Jacelone
        1. Pin-Wheel.png
          21 kB

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Hi, Joseph.

            I tried that in a few combinations, but I saw the expected behaviour, regardless of browser.

            Have you tried this with different users? Have you tried changing the Ajax setting in the user's profile?

            Are you using a theme other than the Standard theme?

            Show
            Michael de Raadt added a comment - Hi, Joseph. I tried that in a few combinations, but I saw the expected behaviour, regardless of browser. Have you tried this with different users? Have you tried changing the Ajax setting in the user's profile? Are you using a theme other than the Standard theme?
            Hide
            Michael de Raadt added a comment -

            Also, when you view the JavaScript console in FF or Chrome, can you see any JavaScript error messages?

            Show
            Michael de Raadt added a comment - Also, when you view the JavaScript console in FF or Chrome, can you see any JavaScript error messages?
            Hide
            Michael de Raadt added a comment -

            The following error message was provided in a duplicate issue...

            Fehler: Y.io is not a function
            Quelldatei: /theme/yui_combo.php?moodle/474/filter_glossary/autolinker/autolinker.js
            Zeile: 62

            Show
            Michael de Raadt added a comment - The following error message was provided in a duplicate issue... Fehler: Y.io is not a function Quelldatei: /theme/yui_combo.php?moodle/474/filter_glossary/autolinker/autolinker.js Zeile: 62
            Hide
            Joseph Jacelone added a comment - - edited

            Dear Michael,

            Sorry I have not been more attentive to this issue. I have also had the same error when viewing the Error Console in Firefox:

            Timestamp: 4/10/2012 7:17:14 AM
            Error: Y.io is not a function
            Source File: http://cmpcourse.com/theme/yui_combo.php?moodle/293/filter_glossary/autolinker/autolinker.js
            Line: 62

            I am attaching the output from Firebug as well. If you would like me to test out anything else let me know. I will be more available the next two weeks.

            Show
            Joseph Jacelone added a comment - - edited Dear Michael, Sorry I have not been more attentive to this issue. I have also had the same error when viewing the Error Console in Firefox: Timestamp: 4/10/2012 7:17:14 AM Error: Y.io is not a function Source File: http://cmpcourse.com/theme/yui_combo.php?moodle/293/filter_glossary/autolinker/autolinker.js Line: 62 I am attaching the output from Firebug as well. If you would like me to test out anything else let me know. I will be more available the next two weeks.
            Hide
            Michael de Raadt added a comment -

            Thanks for providing that additional information.

            Show
            Michael de Raadt added a comment - Thanks for providing that additional information.
            Hide
            Nicolas Martignoni added a comment - - edited

            Also suffering this with Moodle 2.2.2.

            The bug doesn't occur on the course page: the Ajax "window" is opened correctly when clicking on the concept. The source code of the page shows this YUI code:

            Y.use("moodle-filter_glossary-autolinker",function() {
              M.filter_glossary.init_filter_autolinking({"courseid":"130"});
            });
            

            But inside the description of an assignment in the same course, the link is correctly displayed, but the Ajax "window" doesn't open when clicking on the concept (but opens if explicitly requesting in a new tab or new browser window). The code source doesn't have the above code.

            Suspecting that the absence of this JS code is the reason of the bug.

            Show
            Nicolas Martignoni added a comment - - edited Also suffering this with Moodle 2.2.2. The bug doesn't occur on the course page: the Ajax "window" is opened correctly when clicking on the concept. The source code of the page shows this YUI code: Y.use( "moodle-filter_glossary-autolinker" , function () { M.filter_glossary.init_filter_autolinking({ "courseid" : "130" }); }); But inside the description of an assignment in the same course, the link is correctly displayed, but the Ajax "window" doesn't open when clicking on the concept (but opens if explicitly requesting in a new tab or new browser window). The code source doesn't have the above code. Suspecting that the absence of this JS code is the reason of the bug.
            Hide
            Nicolas Martignoni added a comment -

            Also: the bug title should be modified, as it's not a browser window/tab problem, but a "fake" window produced with a yui3-overlay widget. Moreover, the bug doesn't depend on the (modern) browser type (tested with Chrome, Firefox and Safari on Mac OS X).

            I'm afraid that my english is not good enough to change the title to a sensible title, sorry

            Show
            Nicolas Martignoni added a comment - Also: the bug title should be modified, as it's not a browser window/tab problem, but a "fake" window produced with a yui3-overlay widget. Moreover, the bug doesn't depend on the (modern) browser type (tested with Chrome, Firefox and Safari on Mac OS X). I'm afraid that my english is not good enough to change the title to a sensible title, sorry
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I've been brave enough and tidied the title + raised priority. Will be looking to this later today.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I've been brave enough and tidied the title + raised priority. Will be looking to this later today. Ciao
            Hide
            Dan Poltawski added a comment -

            Andrew/Ruslan, adding you here, because I seem to remember we came across this problem, probably ended up not creating a bug - but just inn case we did..

            Show
            Dan Poltawski added a comment - Andrew/Ruslan, adding you here, because I seem to remember we came across this problem, probably ended up not creating a bug - but just inn case we did..
            Hide
            Andrew Nicols added a comment -

            IIRC, we saw this on filters too yes - I can't remember the exact details but I think:

            • if a piece of text has not been cached, it all works;
            • if a piece of text is in the cache, the cache is used, but JS is not.

            I don't think we created a bug at the time as we thought it was an issue with our filter.

            Show
            Andrew Nicols added a comment - IIRC, we saw this on filters too yes - I can't remember the exact details but I think: if a piece of text has not been cached, it all works; if a piece of text is in the cache, the cache is used, but JS is not. I don't think we created a bug at the time as we thought it was an issue with our filter.
            Hide
            Andrew Nicols added a comment -

            I remembered what our issue was now. We were writing a filter which instantiated a jwplayer instance. The way we originally wrote it, each video instance on the page needed to be instantiated once. As a result, when the text came from the cache, the filter didn't need to insert any instances of the player and the JS was never initialized.

            In the end, we turned off text caching rather than fixing the issue. At some point, I'll alter it to only initialize once.

            I had a quick look at the glossary filter. It doesn't work in quite the same way - it always adds the JS init code if there's at least one concept in the glossary, and it hasn't already been added. Since the filter is invoked by filter_text() and it's only invoked if there isn't a match in the text cache, I can see how this problem would arrive. I did have a quite poke around but I haven't successfully been able to replicate the issue so far.

            Show
            Andrew Nicols added a comment - I remembered what our issue was now. We were writing a filter which instantiated a jwplayer instance. The way we originally wrote it, each video instance on the page needed to be instantiated once. As a result, when the text came from the cache, the filter didn't need to insert any instances of the player and the JS was never initialized. In the end, we turned off text caching rather than fixing the issue. At some point, I'll alter it to only initialize once. I had a quick look at the glossary filter. It doesn't work in quite the same way - it always adds the JS init code if there's at least one concept in the glossary, and it hasn't already been added. Since the filter is invoked by filter_text() and it's only invoked if there isn't a match in the text cache, I can see how this problem would arrive. I did have a quite poke around but I haven't successfully been able to replicate the issue so far.
            Hide
            Tim Hunt added a comment -

            I just had a quick look at Eloy's patch. +1 from me.

            Show
            Tim Hunt added a comment - I just had a quick look at Eloy's patch. +1 from me.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Adding fix proposal to master (2.3):

            • Extend the filter_manager() API to support one setup_page_for_filters() method to be invoked before filtering happens.
            • That method will call each active filter setup() method.
            • setup() will be in charge of performing page modifications, controlling execution cardinality and any other init stuff.
            • Take rid of one unused variable, since 2.2, in the yui module.

            Now going to imagine if something can be done for 2.1 and 2.2 (stables), this really is not backportable there.

            Ciao

            PS: Thanks Tim for your support.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Adding fix proposal to master (2.3): Extend the filter_manager() API to support one setup_page_for_filters() method to be invoked before filtering happens. That method will call each active filter setup() method. setup() will be in charge of performing page modifications, controlling execution cardinality and any other init stuff. Take rid of one unused variable, since 2.2, in the yui module. Now going to imagine if something can be done for 2.1 and 2.2 (stables), this really is not backportable there. Ciao PS: Thanks Tim for your support.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Adding possible solution for 22_STABLE. It's achieved by abusing the hash() method to do both the setup and the original (parent) hash().

            Really ugly, but temporary (API already supports it in 2.3) and better than other solutions like adding it always. Read commit comments/phpdocs.

            Also, this is not backportable to 21_STABLE. The glossary filter there was a legacy one and had not support for hash() to be abused. Only workaround there is to disable text caching or upgrade to 2.2/2.3.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Adding possible solution for 22_STABLE. It's achieved by abusing the hash() method to do both the setup and the original (parent) hash(). Really ugly, but temporary (API already supports it in 2.3) and better than other solutions like adding it always. Read commit comments/phpdocs. Also, this is not backportable to 21_STABLE. The glossary filter there was a legacy one and had not support for hash() to be abused. Only workaround there is to disable text caching or upgrade to 2.2/2.3. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sending to integration with the commented fixes for 22 and master. Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Sending to integration with the commented fixes for 22 and master. Ciao
            Hide
            Dan Poltawski added a comment -

            Adding Sam Marshall here, because..

            I suppose this change would allow us to include the media embedding JS only when the media filter is enabled/file resource. That might be beneficial - just an idea.

            Show
            Dan Poltawski added a comment - Adding Sam Marshall here, because.. I suppose this change would allow us to include the media embedding JS only when the media filter is enabled/file resource. That might be beneficial - just an idea.
            Hide
            Dan Poltawski added a comment -

            I've integrated this now, thanks Eloy

            Show
            Dan Poltawski added a comment - I've integrated this now, thanks Eloy
            Hide
            Dan Poltawski added a comment -

            Adrian ran into an error when we had 'filter titles' enabled, I pushed a fix.

            Show
            Dan Poltawski added a comment - Adrian ran into an error when we had 'filter titles' enabled, I pushed a fix.
            Hide
            Dan Poltawski added a comment -

            updated test instruction to include what was breaking

            Show
            Dan Poltawski added a comment - updated test instruction to include what was breaking
            Hide
            Dan Poltawski added a comment -

            Passed - however while testing I noticed that the JS string is also linking to itself, will create a new issue for that

            Show
            Dan Poltawski added a comment - Passed - however while testing I noticed that the JS string is also linking to itself, will create a new issue for that
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Hi,

            crap, thanks for the filterall fix in format_string(). I just added the line and forgot to check it under filterall.

            One more thing, I'm goimg to send straight this change in format_string():

             
            @@ -1303,8 +1303,8 @@ function format_string($string, $striplinks = true, $options = NULL) {
             
                 if (!empty($CFG->filterall)) {
                     $filtermanager = filter_manager::instance();
            -        $string = $filtermanager->filter_string($string, $options['context']);
                     $filtermanager->setup_page_for_filters($PAGE, $options['context']); // Setup global stuff filters may have.
            +        $string = $filtermanager->filter_string($string, $options['context']);
                 }
            

            So setup happens BEFORE filtering. It does not affect much to this glossary case, but has sense to setup (connect to xxxx, whatever) before filtering happens.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Hi, crap, thanks for the filterall fix in format_string(). I just added the line and forgot to check it under filterall. One more thing, I'm goimg to send straight this change in format_string():   @@ -1303,8 +1303,8 @@ function format_string($string, $striplinks = true, $options = NULL) { if (!empty($CFG->filterall)) { $filtermanager = filter_manager::instance(); - $string = $filtermanager->filter_string($string, $options['context']); $filtermanager->setup_page_for_filters($PAGE, $options['context']); // Setup global stuff filters may have. + $string = $filtermanager->filter_string($string, $options['context']); } So setup happens BEFORE filtering. It does not affect much to this glossary case, but has sense to setup (connect to xxxx, whatever) before filtering happens. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Tiny change above applied and retested. Continues working as expected...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Tiny change above applied and retested. Continues working as expected...ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Re: Yes, I was looking to which filters were adding stuff like that and only glossary and mediaplugin were. In fact I've MDL-30624 under the radar, but it's down in my prioritized list right now.

            Show
            Eloy Lafuente (stronk7) added a comment - Re: Yes, I was looking to which filters were adding stuff like that and only glossary and mediaplugin were. In fact I've MDL-30624 under the radar, but it's down in my prioritized list right now.
            Hide
            Sam Marshall added a comment -

            I didn't really follow this or have time to look at it in detail, but basically if you are considering changing the 'whether or not to include JavaScript' thing, please consider the following scenario:

            1. User views page in context where filter X is enabled, but which does not initially contain any content affected by filter X.

            2. As part of JavaScript code, the page makes an AJAX request which requests a new chunk of content (basically, a <div>) to add to the page. This chunk includes new formatted text, and potentially includes content affected by filter X.

            3. There should be a mechanism for adding this chunk of content to the current page so that it operates correctly, and any JavaScript that is required is included.

            There are basically two ways to solve this, (a) make sure that if filter X is enabled in a context, all pages in that context contain any JavaScript required by filter X, or (b) do something complicated so that any JavaScript required by the filter is defined as part of the AJAX response, and has processing to handle it in the code that receives the AJAX data.

            I think (a) is the preferable option.

            (Also note - all JavaScript files should be served with a version number/hash and in such a way that they expire at least 1 year in the future, so the browser does not even have to make an If-Modified-Since request; this will minimise performance implications of including 'extra' JS files. Not quite certain Moodle JS already does this, but assuming it does, including a few small JS scripts for filters shouldn't be end of world.)

            ForumNG retrieves new content for page using AJAX which is why I am concerned about it, but it's not exactly an uncommon thing to do if writing any dynamic software, so I can imagine it being used in other places too.

            NOTE: I am definitely not saying that existing code prior to this change works in that scenario, so it may well be broken before anyhow. Just that it should be considered for anything relating to JS for filters.

            Show
            Sam Marshall added a comment - I didn't really follow this or have time to look at it in detail, but basically if you are considering changing the 'whether or not to include JavaScript' thing, please consider the following scenario: 1. User views page in context where filter X is enabled, but which does not initially contain any content affected by filter X. 2. As part of JavaScript code, the page makes an AJAX request which requests a new chunk of content (basically, a <div>) to add to the page. This chunk includes new formatted text, and potentially includes content affected by filter X. 3. There should be a mechanism for adding this chunk of content to the current page so that it operates correctly, and any JavaScript that is required is included. There are basically two ways to solve this, (a) make sure that if filter X is enabled in a context, all pages in that context contain any JavaScript required by filter X, or (b) do something complicated so that any JavaScript required by the filter is defined as part of the AJAX response, and has processing to handle it in the code that receives the AJAX data. I think (a) is the preferable option. (Also note - all JavaScript files should be served with a version number/hash and in such a way that they expire at least 1 year in the future, so the browser does not even have to make an If-Modified-Since request; this will minimise performance implications of including 'extra' JS files. Not quite certain Moodle JS already does this, but assuming it does, including a few small JS scripts for filters shouldn't be end of world.) ForumNG retrieves new content for page using AJAX which is why I am concerned about it, but it's not exactly an uncommon thing to do if writing any dynamic software, so I can imagine it being used in other places too. NOTE: I am definitely not saying that existing code prior to this change works in that scenario, so it may well be broken before anyhow. Just that it should be considered for anything relating to JS for filters.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao
            Hide
            Joseph Jacelone added a comment -

            Hawt Dog! Thank you all for your hard work!

            Cheers all!!

            Show
            Joseph Jacelone added a comment - Hawt Dog! Thank you all for your hard work! Cheers all!!

              People

              • Votes:
                6 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: