Moodle
  1. Moodle
  2. MDL-17518

Autolinking Filter: Problem autolinking if resource name contains use of double quote (i.e. " )

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.8.7, 1.9.3, 2.0
    • Fix Version/s: 1.8.8, 1.9.4
    • Component/s: Filters
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      30203

      Description

      To demonstrate the bug, create a resource called Article: "Testing" and then create an offline assignment that instructs the student to Read Article: "Testing". I would expect autolinking to work (especially if I copy and paste the resource name into the assignment's description. I've not looked at the code to see what the problem is but I do notice that the description field stores the double quote as " A workaround is to use two single quotes which gives the appearance of a double quote but this is not intuitive to users. This issue is important as teachers who wish to demonstrate good formatting of resource names require the use of quotations (this is how I came across this issue). So getting this patched up would be very helpful. A similar issue occurs when italics are used in the assignment description. Since italics are not allowed in the resource name (they get stripped out if the html is added manually), then I think the text string should also be stripped using the same function prior to doing the compare. Peace - Anthony

      1. MDL-17518.diff
        0.8 kB
        Anthony Borrow
      2. MDL-17518-2.patch.txt
        2 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Eloy - I was taking a quick look at how the resource name is handled and figured I would make a couple notes for myself that also might be useful if you get to this before I do.

          Looking at /mod/resource/mod_form.php the name field for a resource is a form type of either PARAM_TEXT or PARAM_CLEAN depending on whether $CFG->formatstringstriptags is set or not. To test italics, I allowed the activity name to contain the italics tags. Even so, the autolinking did not work. Do you think it would be wise to strip the resource name to PARAM_TEXT before comparing?

          The disadvantage would be that a resource named: My Resource and My <i>Resource</i> would be the same but I would see that more as a benefit. I think this might improve the autolinking and make it a tad more predictable and less fussy.

          I have a day full of classes so will not get to do too much with this today but will continue to plug away at coming up with a patch.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Eloy - I was taking a quick look at how the resource name is handled and figured I would make a couple notes for myself that also might be useful if you get to this before I do. Looking at /mod/resource/mod_form.php the name field for a resource is a form type of either PARAM_TEXT or PARAM_CLEAN depending on whether $CFG->formatstringstriptags is set or not. To test italics, I allowed the activity name to contain the italics tags. Even so, the autolinking did not work. Do you think it would be wise to strip the resource name to PARAM_TEXT before comparing? The disadvantage would be that a resource named: My Resource and My <i>Resource</i> would be the same but I would see that more as a benefit. I think this might improve the autolinking and make it a tad more predictable and less fussy. I have a day full of classes so will not get to do too much with this today but will continue to plug away at coming up with a patch. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Eloy - Can you think of any unintended consequences by using s here in the /mod/resource/filter.php file? This seems to allow the use of double quotes fine. I think the italics issue is something to be handled separately. Peace - Anthony

          Show
          Anthony Borrow added a comment - Eloy - Can you think of any unintended consequences by using s here in the /mod/resource/filter.php file? This seems to allow the use of double quotes fine. I think the italics issue is something to be handled separately. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Based on http://us.php.net/htmlspecialchars (since it is used in s),

          I think I would want to test a variety of scenarios involving the > (>), < (<), & (&), " (") [when ENT_NOQUOTES is not set], and ' (') [ only when ENT_QUOTES is set] characters to make sure we are not breaking things that worked before.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Based on http://us.php.net/htmlspecialchars (since it is used in s), I think I would want to test a variety of scenarios involving the > (>), < (<), & (&), " (") [when ENT_NOQUOTES is not set] , and ' (') [ only when ENT_QUOTES is set] characters to make sure we are not breaking things that worked before. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Eloy - I did some quick testing and here is what I found:

          *Double quote: Without s, does not autolink. The double quote " character is accepted in the resource name and does not get changed to an html entity. Works with s.

          Single quote: Works fine without s. OK when entered as ' in resource name (does not get converted to html entity in resource name). Works with s.

          *Ampersand: Without s, does not autolink. The & character is accepted in the resource name and does not get changed to an html entity. Works with s.

          Greater than: Works fine without s. OK when entered as > in resource name but gets converted to html entity > which causes it not to work with s.

          Less than (html entity required): Works fine without s provided less than is entered as an html entity < otherwise the text is truncated at that point and does not display in the resource name. The html entity gets converted by s and returns &lt; rather than < Since this is truncated by default (without using html entity) I am less concerned with it not working.

          In summary, adding the s call will fix the double quote and ampersand problem (provided those characters are not entered in the resource name as html entities which I think would be less common). It does not effect single quotes (provided they are not entered as html entities in the resource name). If the less than character were to have been entered as an html entity then it works without s but adding s breaks it but considering that the less than character when entered normally causes the resource name to be truncated at that point I would say that it is not an accepted character for a resource name and therefore not a condition we necessarily have to worry about. Similarly for the greater than character which can be entered in the resource name but automatically gets converted to an html entity. In that case, without s it works and the s breaks it. I think if we were to document that the resource name should not contain < or > characters we would be OK. Given that the ampersand and double quotes are more likely to be used, I think this would fix more problems than it causes but I leave it to your judgment to sort out how best to deal with this.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Eloy - I did some quick testing and here is what I found: *Double quote: Without s, does not autolink. The double quote " character is accepted in the resource name and does not get changed to an html entity. Works with s. Single quote: Works fine without s. OK when entered as ' in resource name (does not get converted to html entity in resource name). Works with s. *Ampersand: Without s, does not autolink. The & character is accepted in the resource name and does not get changed to an html entity. Works with s. Greater than: Works fine without s. OK when entered as > in resource name but gets converted to html entity > which causes it not to work with s. Less than (html entity required): Works fine without s provided less than is entered as an html entity < otherwise the text is truncated at that point and does not display in the resource name. The html entity gets converted by s and returns &lt; rather than < Since this is truncated by default (without using html entity) I am less concerned with it not working. In summary, adding the s call will fix the double quote and ampersand problem (provided those characters are not entered in the resource name as html entities which I think would be less common). It does not effect single quotes (provided they are not entered as html entities in the resource name). If the less than character were to have been entered as an html entity then it works without s but adding s breaks it but considering that the less than character when entered normally causes the resource name to be truncated at that point I would say that it is not an accepted character for a resource name and therefore not a condition we necessarily have to worry about. Similarly for the greater than character which can be entered in the resource name but automatically gets converted to an html entity. In that case, without s it works and the s breaks it. I think if we were to document that the resource name should not contain < or > characters we would be OK. Given that the ampersand and double quotes are more likely to be used, I think this would fix more problems than it causes but I leave it to your judgment to sort out how best to deal with this. Peace - Anthony
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Anthony...

          I've been examining a bit the proposal and sounds correct...but... there is an important drawback and it's that people not using the HTML Editor... don't get those quotes converted to " entities (same for other conversions)... so the change will break things for them

          Perhaps we could add both phrases (the original and the "entitised") to the list of list of resources instead? That way both users using HTML editor and not using it will have those links working).

          One potential problem is that, this way, we are doubling the number of phrases to look for... so I think we'll need to make it a bit clever to only add both when the original and the entitised names are different. That could be a good approach.

          Finally, I think that the activitynames filter can suffer the same problem (after a quick look). We'll need the same approach to be applied there IMO.

          Does this sound ok?

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Anthony... I've been examining a bit the proposal and sounds correct...but... there is an important drawback and it's that people not using the HTML Editor... don't get those quotes converted to " entities (same for other conversions)... so the change will break things for them Perhaps we could add both phrases (the original and the "entitised") to the list of list of resources instead? That way both users using HTML editor and not using it will have those links working). One potential problem is that, this way, we are doubling the number of phrases to look for... so I think we'll need to make it a bit clever to only add both when the original and the entitised names are different. That could be a good approach. Finally, I think that the activitynames filter can suffer the same problem (after a quick look). We'll need the same approach to be applied there IMO. Does this sound ok?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding MDL-17518-2.patch.txt (for 19_STABLE) that implements the behaviour explained above (adding extra filter strings for resources having entities).

          Show
          Eloy Lafuente (stronk7) added a comment - Adding MDL-17518 -2.patch.txt (for 19_STABLE) that implements the behaviour explained above (adding extra filter strings for resources having entities).
          Hide
          Anthony Borrow added a comment -

          Eloy - I tested your patch and it works great for all situations. I liked the idea of creating the entitisedname and comparing it as it covers all scenarios. Peace - Anthony

          Show
          Anthony Borrow added a comment - Eloy - I tested your patch and it works great for all situations. I liked the idea of creating the entitisedname and comparing it as it covers all scenarios. Peace - Anthony
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Patch applied to 18_STABLE, 19_STABLE and HEAD. Resolving as fixed. Going to apply same solution to MDL-17545 (activity names filter).

          Thanks Anthony. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Patch applied to 18_STABLE, 19_STABLE and HEAD. Resolving as fixed. Going to apply same solution to MDL-17545 (activity names filter). Thanks Anthony. Ciao
          Hide
          Tim Hunt added a comment -

          Nice fix. Thanks.

          Show
          Tim Hunt added a comment - Nice fix. Thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: