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

Convert URL into Links filter prevents backgrounds on tables when Apply to HTML Format is checked.

    Details

    • Testing Instructions:
      Hide
      1. Enable the filter to convert URLs to links/images
        • Enable HTML format on its settings
      2. Add a label to a course (use TinyMCE or a textarea with HTML format)
        • Place a URL to an image
        • Place a URL to a website
        • Add a table with a background image (full URL not relative)
      3. Save and confirm that everything is rendered as expected
      Show
      Enable the filter to convert URLs to links/images Enable HTML format on its settings Add a label to a course (use TinyMCE or a textarea with HTML format) Place a URL to an image Place a URL to a website Add a table with a background image (full URL not relative) Save and confirm that everything is rendered as expected
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38698-master
    • Sprint:
      FRONTEND Sprint 7
    • Story Points (Obsolete):
      13
    • Sprint:
      FRONTEND Sprint 7

      Description

      Issue: Convert URL into Links filter prevents backgrounds on tables when Apply to HTML Format is checked.

      Steps to replicate:

      1. Go to: Site Admin>Plugins>filters>Convert URL into Links and Images
      2. Ensure that HTML format is checked
      3. Go to course
      4. Add a Label
      5. Make a table and add a background image using css to add a background
        Save

      Actual Result:
      You will see the link for the background images as text above a table without an image

      Expected result:
      Table with image as background

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put that on the backlog.

            In the meantime, feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a "patch" label so we will spot it.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime, feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a "patch" label so we will spot it.
            Hide
            fred Frédéric Massart added a comment -

            Pushing this for peer review. Here is a solution that ignores the CSS property url(...). The downside of this solution is that some URL wrapped between url() as a text, not a CSS property will not be converted.

            Weighing the good and bad about this solution, I think it's better to have a strong filtering on the CSS property without increasing the complexity of the regex (we need a much complex one to filter based on style="" or background. We could make the rule less strict, by assuming that quotes need to be there in the property, but then there is a risk of converting them then.

            Please let me know what you think.

            Fred

            Show
            fred Frédéric Massart added a comment - Pushing this for peer review. Here is a solution that ignores the CSS property url(...) . The downside of this solution is that some URL wrapped between url() as a text, not a CSS property will not be converted. Weighing the good and bad about this solution, I think it's better to have a strong filtering on the CSS property without increasing the complexity of the regex (we need a much complex one to filter based on style="" or background . We could make the rule less strict, by assuming that quotes need to be there in the property, but then there is a risk of converting them then. Please let me know what you think. Fred
            Hide
            damyon Damyon Wiese added a comment -

            Hi Fred, this seems a bit too specific. Really - we never want to convert urls in any html attributes (including but not limited to style) or within style tags.

            The first one could instead be fixed by a small change to the original regex:

            change
            $regex = "(?<!=[\"'])$urlstart
            to
            $regex = "(?<!=[\"'][^>]+)$urlstart

            The second may be easy to fix by adding style to the ignoretags above (haven't tried it). Maybe we should add <script> too?

            What do you think?

            Show
            damyon Damyon Wiese added a comment - Hi Fred, this seems a bit too specific. Really - we never want to convert urls in any html attributes (including but not limited to style) or within style tags. The first one could instead be fixed by a small change to the original regex: change $regex = "(?<!=[\"'])$urlstart to $regex = "(?<!=[\"'][^>]+)$urlstart The second may be easy to fix by adding style to the ignoretags above (haven't tried it). Maybe we should add <script> too? What do you think?
            Hide
            fred Frédéric Massart added a comment -

            Hi Damyon,

            I would, but lookbehind assertions must have a fixed size.
            Maybe we should as script, but I'd rather discuss this in another issue as it could lead to other problems. If we do not convert what is in the <script ...> tag, we should also handle what is between <script></script>. Are we actually encouraging people to include scripts in their content?

            Was there anything else?

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Damyon, I would, but lookbehind assertions must have a fixed size. Maybe we should as script , but I'd rather discuss this in another issue as it could lead to other problems. If we do not convert what is in the <script ...> tag, we should also handle what is between <script></script>. Are we actually encouraging people to include scripts in their content? Was there anything else? Cheers, Fred
            Hide
            damyon Damyon Wiese added a comment -

            I don't understand your first comment Fred. I am suggesting we don't use your new lookbehindassertions at all - just make that minor regex tweak above. (fine to ignore script tags in this issue - note that teachers are allowed to add javascript - they are trusted).

            Show
            damyon Damyon Wiese added a comment - I don't understand your first comment Fred. I am suggesting we don't use your new lookbehindassertions at all - just make that minor regex tweak above. (fine to ignore script tags in this issue - note that teachers are allowed to add javascript - they are trusted).
            Hide
            fred Frédéric Massart added a comment -

            The assertion your are suggesting to use above does not work. It is a lookbehind assertion and those need a fixed width.

            Wall-E:sm fmc$ php -r "var_dump(preg_replace(\"@(?<!=[\\\"'][^>]+)http://@\", '', 'http://'));"
            PHP Warning:  preg_replace(): Compilation failed: lookbehind assertion is not fixed length at offset 14 in Command line code on line 1
             
            Warning: preg_replace(): Compilation failed: lookbehind assertion is not fixed length at offset 14 in Command line code on line 1
            NULL

            Show
            fred Frédéric Massart added a comment - The assertion your are suggesting to use above does not work. It is a lookbehind assertion and those need a fixed width. Wall-E:sm fmc$ php -r "var_dump(preg_replace(\"@(?<!=[\\\"'][^>]+)http://@\", '', 'http://'));" PHP Warning: preg_replace(): Compilation failed: lookbehind assertion is not fixed length at offset 14 in Command line code on line 1   Warning: preg_replace(): Compilation failed: lookbehind assertion is not fixed length at offset 14 in Command line code on line 1 NULL
            Hide
            damyon Damyon Wiese added a comment -

            Right - not so easy then.

            Your patch fixes the reported issue - it just seems odd to me to only exclude url(http://) from matching when really we should be only matching real text and nothing inside html tags with this filter.

            There are no other issues with this patch - so I'm happy for you to push for integration.

            Show
            damyon Damyon Wiese added a comment - Right - not so easy then. Your patch fixes the reported issue - it just seems odd to me to only exclude url( http:// ) from matching when really we should be only matching real text and nothing inside html tags with this filter. There are no other issues with this patch - so I'm happy for you to push for integration.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Damyon,

            I agree that it might not be the best way, but without a partial rewrite of the filter I cannot think of any other option. The decision to be specific was already made when we ignored anything starting with =" rather than ignoring tags content.

            Pushing for integration now.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks Damyon, I agree that it might not be the best way, but without a partial rewrite of the filter I cannot think of any other option. The decision to be specific was already made when we ignored anything starting with =" rather than ignoring tags content. Pushing for integration now. Cheers, Fred
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hmmmm I've run the unit tests for the urltolink filter 10 times and its failed 6 of those times due to the performance test:

            1) filter_urltolink_testcase::test_convert_urls_into_links_performance
            Timing test: 0.78488111495972secs (new) < 0.71183586120605secs (old)
            Failed asserting that true matches expected false.

            /var/www/integration/filter/urltolink/tests/filter_test.php:213
            /var/www/integration/lib/phpunit/classes/basic_testcase.php:64

            To re-run:
            vendor/bin/phpunit filter_urltolink_testcase filter/urltolink/tests/filter_test.php

            FAILURES!
            Tests: 68, Assertions: 68, Failures: 1.

            Now, that's pretty annoying, why is a performance test in unit tests.
            While not directly related to your changes obviously your changes affect performance enough to start triggering this test.
            After speaking with Eloy we were both in agreement that really that test could/should be removed.
            Would you please remove it as part of your changes here Fred?

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hmmmm I've run the unit tests for the urltolink filter 10 times and its failed 6 of those times due to the performance test: 1) filter_urltolink_testcase::test_convert_urls_into_links_performance Timing test: 0.78488111495972secs (new) < 0.71183586120605secs (old) Failed asserting that true matches expected false. /var/www/integration/filter/urltolink/tests/filter_test.php:213 /var/www/integration/lib/phpunit/classes/basic_testcase.php:64 To re-run: vendor/bin/phpunit filter_urltolink_testcase filter/urltolink/tests/filter_test.php FAILURES! Tests: 68, Assertions: 68, Failures: 1. Now, that's pretty annoying, why is a performance test in unit tests. While not directly related to your changes obviously your changes affect performance enough to start triggering this test. After speaking with Eloy we were both in agreement that really that test could/should be removed. Would you please remove it as part of your changes here Fred? Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Also Eloy noted that perhaps you explicitly mention in your test instructions to use FORMAT_MOODLE, or to change the filters format settings to what is going to be used. Just to ensure this is explicitly tested.

            Show
            samhemelryk Sam Hemelryk added a comment - Also Eloy noted that perhaps you explicitly mention in your test instructions to use FORMAT_MOODLE, or to change the filters format settings to what is going to be used. Just to ensure this is explicitly tested.
            Hide
            fred Frédéric Massart added a comment -

            Hi Sam,

            I had noticed this test, but I managed to have it pass on my computer. That was actually interesting as I changed a bit my regex to make it pass. Anyway, I have removed it fails for you, and is quite unreliable anyway.

            I have also updated the testing instructions, I had forgotten about the HTML format. Thanks for reminding me.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Sam, I had noticed this test, but I managed to have it pass on my computer. That was actually interesting as I changed a bit my regex to make it pass. Anyway, I have removed it fails for you, and is quite unreliable anyway. I have also updated the testing instructions, I had forgotten about the HTML format. Thanks for reminding me. Cheers, Fred
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks for making those changes Fred, this has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for making those changes Fred, this has been integrated now
            Hide
            phalacee Jason Fowler added a comment -

            All good Fred, thanks for that

            Show
            phalacee Jason Fowler added a comment - All good Fred, thanks for that
            Hide
            damyon Damyon Wiese added a comment -

            Twas the week before Christmas,
            And all though HQ
            Devs were scrambling to finish peer review.
            They sent all their issues,
            and rushed out the door -
            "To the beach!" someone heard them roar!

            This issue has been released upstream. Thanks!

            Show
            damyon Damyon Wiese added a comment - Twas the week before Christmas, And all though HQ Devs were scrambling to finish peer review. They sent all their issues, and rushed out the door - "To the beach!" someone heard them roar! This issue has been released upstream. Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14

                  Agile