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

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