Moodle
  1. Moodle
  2. MDL-28168

Custom menu output broken with urltolink filter enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1, 2.2
    • Fix Version/s: 2.0.4, 2.1.1
    • Component/s: Filters
    • Labels:
    • Testing Instructions:
      Hide

      Testing for 2.0:

      • Multi-language custom menus not supported on 2.0. Reverting the previous buggy implementation. Just make sure the menu works as it used to.

      Testing for 2.1:

      • Install another language pack to the site (for example Czech - Čeština "cs")
      • Enable multi/language filter (content filtering is enough)
      • Go to Site administration > Appearance > Themes > Theme settings and define the custom menu using a multi-lang syntax (see comments below for some example using the English/Czech variants of the menu)
      • TEST: make sure that when switching the language, you get relevant custom menu displayed
      Show
      Testing for 2.0: Multi-language custom menus not supported on 2.0. Reverting the previous buggy implementation. Just make sure the menu works as it used to. Testing for 2.1: Install another language pack to the site (for example Czech - Čeština "cs") Enable multi/language filter (content filtering is enough) Go to Site administration > Appearance > Themes > Theme settings and define the custom menu using a multi-lang syntax (see comments below for some example using the English/Czech variants of the menu) TEST: make sure that when switching the language, you get relevant custom menu displayed
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28168-custommenu
    • Rank:
      17872

      Description

      This is one regression caused by MDL-27073.

      To reproduce:

      • Into any 2.0/2.1/master site enable the urtolink filter (active for MOODLE_FORMAT)
      • Define one custom menu with full links, mine was, for example:
      Switch to postgres|http://127.0.0.1/~stronk7/git?dbconfig=postgres
      Switch to mssql|http://127.0.0.1/~stronk7/git?dbconfig=mssql
      Switch to oracle|http://127.0.0.1/~stronk7/git?dbconfig=oracle
      
      Debug config|http://127.0.0.1/~stronk7/git?debugconfig
      (using db mysql, git branch MOODLE_21_STABLE, based on master)|
      
      • Take a look to the custom menu generated, all the links are borked. The cause is the urltolink detecting those URLs (because we changed to format_text()) and putting them within <a> tags, completely breaking the process.

      Note this affects every site using custom menus and having that filter enabled for the MOODLE_FORMAT.

      Workarounds: Disable the filter or make it not to process MOODLE_FORMAT (but it's one of the logical formats to process).

      Possible fixes:

      1) Programatically enclose the links under <nolink> sections before calling format_text()
      2) Change the format in the call from FORMAT_MOODLE to another less suitable to have that filter enabled (FORMAT_HTML?).
      3) Change the the format_text() call to format_string(), set the urltolink filter to process only contents (not strings) and require to enable filterall.

      My +1 goes to 1) above. Ciao

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          What are the chances of getting method 1. (as put forward in MDL-27073) implemented instead?

          This is the suggestion:1) We could use a notation like this:

          [en-US:Moodle community;pt-PT:Comunidade Moodle]|http://moodle.org
          -[en-US:Moodle free support;pt-PT:Suporte gratuito do Moodle]|http://moodle.org/support
          -[en-US:Moodle development;pt-PT:Desenvolvimento do Moodle]|http://moodle.org/development

          by adding in /lib/outputcomponents.php (around line 2386) something like:
          if (!array_key_exists(0, $bits) || empty($bits[0]))

          { // Every item must have a name to be valid continue; }

          else {
          $bits[0] = ltrim($bits[0],'-');

          + $languages = explode(';', $bits[0]); // pt:nome;en:name|url|title|sort
          + if (array_key_exists(0, $languages)) {
          + foreach ($languages as $language)

          { + $menulang = explode(':', $language); + if ($menulang[0] == current_language() ) + $bits[0] = $menulang[1]; + }

          + }

          I personally found this much easier to use. You don't need to add the square brackets either.
          For example:

          en:Moodle community;pt:Comunidade Moodle]|http://moodle.org
          is really easy to write and works. The PHP code is less intrusive too.

          Mary

          Show
          Mary Evans added a comment - What are the chances of getting method 1. (as put forward in MDL-27073 ) implemented instead? This is the suggestion:1) We could use a notation like this: [en-US:Moodle community;pt-PT:Comunidade Moodle] |http://moodle.org - [en-US:Moodle free support;pt-PT:Suporte gratuito do Moodle] |http://moodle.org/support - [en-US:Moodle development;pt-PT:Desenvolvimento do Moodle] |http://moodle.org/development by adding in /lib/outputcomponents.php (around line 2386) something like: if (!array_key_exists(0, $bits) || empty($bits [0] )) { // Every item must have a name to be valid continue; } else { $bits [0] = ltrim($bits [0] ,'-'); + $languages = explode(';', $bits [0] ); // pt:nome;en:name|url|title|sort + if (array_key_exists(0, $languages)) { + foreach ($languages as $language) { + $menulang = explode(':', $language); + if ($menulang[0] == current_language() ) + $bits[0] = $menulang[1]; + } + } I personally found this much easier to use. You don't need to add the square brackets either. For example: en:Moodle community;pt:Comunidade Moodle]|http://moodle.org is really easy to write and works. The PHP code is less intrusive too. Mary
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, after trying the 1) above (add <nolink> sections to the URLs) it seems that not all filters support that nor it is handled in a global way, so the urltolink filter continued doing its job.

          So, based on that, there are 2 options:

          A- Or we make <nolink> to be supported and applied globally.
          B- Or we make the urltolink to support <nolink>
          C- Or we try to preserve those URLs in a different way

          So, I followed the C route, although I think A) is the best for the future.

          The patches below (for 2.0, 2.1 and master) just extract all the URLs from the custommenu contents, replacing them by safe placeholders and, after calling to format_text() puts the the original URLs back.

          For your consideration:

          master: https://github.com/stronk7/moodle/compare/master...MDL-28168_master
          21_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_21_STABLE...MDL-28168_m21
          20_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_20_STABLE...MDL-28168_m20

          Sure it needs testing with custom menus with 3 & 4 element but it's working for the example above. Let's break it!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, after trying the 1) above (add <nolink> sections to the URLs) it seems that not all filters support that nor it is handled in a global way, so the urltolink filter continued doing its job. So, based on that, there are 2 options: A- Or we make <nolink> to be supported and applied globally. B- Or we make the urltolink to support <nolink> C- Or we try to preserve those URLs in a different way So, I followed the C route, although I think A) is the best for the future. The patches below (for 2.0, 2.1 and master) just extract all the URLs from the custommenu contents, replacing them by safe placeholders and, after calling to format_text() puts the the original URLs back. For your consideration: master: https://github.com/stronk7/moodle/compare/master...MDL-28168_master 21_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_21_STABLE...MDL-28168_m21 20_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_20_STABLE...MDL-28168_m20 Sure it needs testing with custom menus with 3 & 4 element but it's working for the example above. Let's break it! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Ho Mary,

          np here with the syntax selected. The only "negative" point I see about your alternative... is that it's a "new" syntax (never used before). And then people will ask... "and why cannot we introduce that syntax for the whole Moodle?" and the ball will roll and roll...

          That's looking the proposal in the whole context. Surely if I look to it in an isolated way I'd say it's clearer and simpler than the current one (although the current one is the "standard" one for any multilang content).

          Ah, great, just one idea... what if somebody does one filter that, simply, gets $CFG->custommenuitems and transforms it from your syntax to the "standard" one? Nah.

          In any case, I don't want to discuss / decide about that, feel free to continue the discussion here / forums / wherever, others will argue / opine .... sure!

          My interest right now is, exclusively, to fix the existing problem ASAP, as far as it can be breaking those URLs in a lot of 2.0 / 2.1 sites.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Ho Mary, np here with the syntax selected. The only "negative" point I see about your alternative... is that it's a "new" syntax (never used before). And then people will ask... "and why cannot we introduce that syntax for the whole Moodle?" and the ball will roll and roll... That's looking the proposal in the whole context. Surely if I look to it in an isolated way I'd say it's clearer and simpler than the current one (although the current one is the "standard" one for any multilang content). Ah, great, just one idea... what if somebody does one filter that, simply, gets $CFG->custommenuitems and transforms it from your syntax to the "standard" one? Nah. In any case, I don't want to discuss / decide about that, feel free to continue the discussion here / forums / wherever, others will argue / opine .... sure! My interest right now is, exclusively, to fix the existing problem ASAP, as far as it can be breaking those URLs in a lot of 2.0 / 2.1 sites. Ciao
          Hide
          David Mudrak added a comment -

          I would follow "C- Or we try to preserve those URLs in a different way" in yet another different way which would be more robust. Let us introduce a new $option for the format_text() that would contain a list of allowed filters. In this situation, we would allow just multilang. The format_text() would pass the option to filter_manager::filter_text() which would pass it to apply_filter_chain() and there just a few lines to be added (simple if in_array() or so).

          Note that this new option could be use anywhere else where we need to make sure that only some required filters are applied an not the others.

          Show
          David Mudrak added a comment - I would follow "C- Or we try to preserve those URLs in a different way" in yet another different way which would be more robust. Let us introduce a new $option for the format_text() that would contain a list of allowed filters. In this situation, we would allow just multilang. The format_text() would pass the option to filter_manager::filter_text() which would pass it to apply_filter_chain() and there just a few lines to be added (simple if in_array() or so). Note that this new option could be use anywhere else where we need to make sure that only some required filters are applied an not the others.
          Hide
          Sam Hemelryk added a comment -

          Linking to an issue I created when I noticed this on moodle.org
          Presently these is a hack on Moodle.org which was made to allow us to go live with it after 2.1 upgrade.

          Show
          Sam Hemelryk added a comment - Linking to an issue I created when I noticed this on moodle.org Presently these is a hack on Moodle.org which was made to allow us to go live with it after 2.1 upgrade.
          Hide
          Petr Škoda added a comment -

          I do not like 1,2,3, A, B. The problem is that custom text is not html - our filters can never work with that reliably. I think we should not use format_text(), format_string() or filters there.

          My +1 for some custom lang processing maybe with custom lang syntax, the multilang may evolve or somebody may use own plugin for that. custommenu seems to be a line based configuration format that can not be wrapped in multilang html tags easily.

          Show
          Petr Škoda added a comment - I do not like 1,2,3, A, B. The problem is that custom text is not html - our filters can never work with that reliably. I think we should not use format_text(), format_string() or filters there. My +1 for some custom lang processing maybe with custom lang syntax, the multilang may evolve or somebody may use own plugin for that. custommenu seems to be a line based configuration format that can not be wrapped in multilang html tags easily.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Guys, decide whatever you want: new custom processing, A, B, C, 1, 2... Ñ or W :-P (The fact right now it that any site with custom menus + linktourl enabled is 100% borked from 20_STABLE to master.

          I just explored some options and proposed one (simple) solution for fun. So, feel free, but ASAP

          Ciao

          Offtopic: I'm not sure if I want multilang having to support more and more syntaxes at all. It is already supporting lang, span... so 100x100 custom processing here if new syntax is decided. And be ready, tomorrow someone will want that syntax to be applied and working elsewhere.

          Show
          Eloy Lafuente (stronk7) added a comment - Guys, decide whatever you want: new custom processing, A, B, C, 1, 2... Ñ or W :-P (The fact right now it that any site with custom menus + linktourl enabled is 100% borked from 20_STABLE to master. I just explored some options and proposed one (simple) solution for fun. So, feel free, but ASAP Ciao Offtopic: I'm not sure if I want multilang having to support more and more syntaxes at all. It is already supporting lang, span... so 100x100 custom processing here if new syntax is decided. And be ready, tomorrow someone will want that syntax to be applied and working elsewhere.
          Hide
          David Mudrak added a comment -

          I'm on it as it was regression of my patch.

          Show
          David Mudrak added a comment - I'm on it as it was regression of my patch.
          Hide
          David Mudrak added a comment -

          Hi. I have a working patch fixing this. Can you please quickly review the suggested way.

          After discussing with Sam H. (the author of custom_menu feature) we agreed that the following small improvement should be done together with this:

          The custom_menu constructor should not accept the useless $text param. Instead, it accepts the menu definition now and optionally the user's current language. This allows to write unit tests for custom_menu as it is not dependent on $CFG->custommenuitems any more (surely that $CFG is just an implementation details of where the menu is defined).

          The multilang support is re-implemented in the following way:

          • the menu items (lines in the $CFG->custommenuitems) can now contain the 3rd optional part with the explicit list of languages the item is valid for
          • menu item without the language specified is valid for any language (backwards compatible)
          • menu item with some language(s) specified is used if and only if the user's current language is one of those explicitly declared for that menu item

          See the added unit tests for the usage examples.

          Show
          David Mudrak added a comment - Hi. I have a working patch fixing this. Can you please quickly review the suggested way. After discussing with Sam H. (the author of custom_menu feature) we agreed that the following small improvement should be done together with this: The custom_menu constructor should not accept the useless $text param. Instead, it accepts the menu definition now and optionally the user's current language. This allows to write unit tests for custom_menu as it is not dependent on $CFG->custommenuitems any more (surely that $CFG is just an implementation details of where the menu is defined). The multilang support is re-implemented in the following way: the menu items (lines in the $CFG->custommenuitems) can now contain the 3rd optional part with the explicit list of languages the item is valid for menu item without the language specified is valid for any language (backwards compatible) menu item with some language(s) specified is used if and only if the user's current language is one of those explicitly declared for that menu item See the added unit tests for the usage examples.
          Hide
          Sam Hemelryk added a comment -

          Hi David,
          Looks great to me!

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi David, Looks great to me! Cheers Sam
          Hide
          David Mudrak added a comment -

          Submitting patches for 2.0, 2.1 and 2.2dev

          • For 2.0, the patch reverts the previous format_text() based implementation. 2.0 will not support multi-language custom menus
          • For 2.1 and 2.2dev, the patch reverts the previous implementation and re-implements the custom menu parsers as agreed with Sam H.
          Show
          David Mudrak added a comment - Submitting patches for 2.0, 2.1 and 2.2dev For 2.0, the patch reverts the previous format_text() based implementation. 2.0 will not support multi-language custom menus For 2.1 and 2.2dev, the patch reverts the previous implementation and re-implements the custom menu parsers as agreed with Sam H.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Not 100% sure about the full revert in 20_STABLE, but...

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Not 100% sure about the full revert in 20_STABLE, but...
          Hide
          Sam Hemelryk added a comment -

          Thanks guys have given this a good test now and certainly a pass!

          Show
          Sam Hemelryk added a comment - Thanks guys have given this a good test now and certainly a pass!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          All this cool stuff is now part of Moodle, the beast! Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - All this cool stuff is now part of Moodle, the beast! Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: