Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Component/s: moodle.org
    • Labels:
      None
    • Rank:
      25787

      Description

      see attchments to understand this issue
      or browse the page http://moodle.org/mod/forum/discuss.php?d=149803

        Issue Links

          Activity

          Hide
          Helen Foster added a comment -

          Daniele, thanks for your report with screenshot illustrating the problem. Assigning to Sam.

          Show
          Helen Foster added a comment - Daniele, thanks for your report with screenshot illustrating the problem. Assigning to Sam.
          Hide
          Sam Hemelryk added a comment -

          Thanks for the report Daniele.
          I've confirmed the problem, it is being caused by a clash between autolinking and the syntax highlighting.
          I'll start looking into this shortly.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for the report Daniele. I've confirmed the problem, it is being caused by a clash between autolinking and the syntax highlighting. I'll start looking into this shortly. Cheers Sam
          Hide
          David Mudrak added a comment -

          Sam is right. The problem is there because of the current text filtering logic:

          1. Forum module calls format_text() when displaying the post message
          2. The message text is converted into HTML by s() or markdown_to_html() or text_to_html() according the text format used for the post
          3. The message text in HTML format is cleaned up by clean_text()
          4. The chain of filters is applied to the message text

          Now, when the post is in FORMAT_MOODLE (as in the case of attached example), text_to_html() is called in step 2. This function calls convert_urls_into_links() and so GeSHi filter has no chance to get to the original text without <a>

          From my point of view, there is the only way how to get around this: get rid of convert_urls_into_links(), move it from core and transform it to a normal filter. Then admin would just set it as the last element in the filter chain and GeSHi would work correctly (tested on my own machine).

          There is an issue, though. Currently, filters are not informed about the format in which the text was originally written - because at the moment they can be applied, the text is in HTML only. Therefore the new filter_urltolink would apply to all other formats, too. That breaks backwards compatibility as only URLs in FORMAT_MOODLE texts are converted now. But it is not that hard to extend our current filters API and provide them the information of the format.

          Please note that the same logic should apply to replace_smilies() - it should be converted into ordinary filter, too.

          I'll be happy to work on this. Should not take more than one day as the code and unit tests are already written.

          Show
          David Mudrak added a comment - Sam is right. The problem is there because of the current text filtering logic: 1. Forum module calls format_text() when displaying the post message 2. The message text is converted into HTML by s() or markdown_to_html() or text_to_html() according the text format used for the post 3. The message text in HTML format is cleaned up by clean_text() 4. The chain of filters is applied to the message text Now, when the post is in FORMAT_MOODLE (as in the case of attached example), text_to_html() is called in step 2. This function calls convert_urls_into_links() and so GeSHi filter has no chance to get to the original text without <a> From my point of view, there is the only way how to get around this: get rid of convert_urls_into_links(), move it from core and transform it to a normal filter. Then admin would just set it as the last element in the filter chain and GeSHi would work correctly (tested on my own machine). There is an issue, though. Currently, filters are not informed about the format in which the text was originally written - because at the moment they can be applied, the text is in HTML only. Therefore the new filter_urltolink would apply to all other formats, too. That breaks backwards compatibility as only URLs in FORMAT_MOODLE texts are converted now. But it is not that hard to extend our current filters API and provide them the information of the format. Please note that the same logic should apply to replace_smilies() - it should be converted into ordinary filter, too. I'll be happy to work on this. Should not take more than one day as the code and unit tests are already written.
          Hide
          Martin Dougiamas added a comment -

          This makes sense to me. +1 for that

          BUT how do you propose to extend the filter API?

          We need some kind of named parameters (eg object passed) because I think there were other similar problems needing solving too.

          I'd also like to see some backward compatibility for old filters.

          Show
          Martin Dougiamas added a comment - This makes sense to me. +1 for that BUT how do you propose to extend the filter API? We need some kind of named parameters (eg object passed) because I think there were other similar problems needing solving too. I'd also like to see some backward compatibility for old filters.
          Hide
          David Mudrak added a comment -

          Martin, the proposal to extend the filter_manager API is tracked in MDL-24531 together with a patch illustrating it. If we do not extend the API (so that the original format is passed to the filter), the new filter_urltolink would have to be applied to all formats and that would break BC (because currently it converts URLs only in FORMAT_MOODLE). Therefore I set MDL-24531 as the blocker for this issue. If the patch proposed in the blocker is accepted, I have working fix for this issue too.

          Show
          David Mudrak added a comment - Martin, the proposal to extend the filter_manager API is tracked in MDL-24531 together with a patch illustrating it. If we do not extend the API (so that the original format is passed to the filter), the new filter_urltolink would have to be applied to all formats and that would break BC (because currently it converts URLs only in FORMAT_MOODLE). Therefore I set MDL-24531 as the blocker for this issue. If the patch proposed in the blocker is accepted, I have working fix for this issue too.
          Hide
          David Mudrak added a comment -

          Hi Sam,

          this should be now solved. I just committed new filter urltolink. At moodle.org, please activate that filter and make sure that it is applied after the GeSHi filter (it makes sense to me to have urltolink one of the last filters in the chain).

          Show
          David Mudrak added a comment - Hi Sam, this should be now solved. I just committed new filter urltolink. At moodle.org, please activate that filter and make sure that it is applied after the GeSHi filter (it makes sense to me to have urltolink one of the last filters in the chain).

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development