Moodle
  1. Moodle
  2. MDL-38055

Navigation block doesn't handle ampersands in course name correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Navigation
    • Labels:
    • Rank:
      47852

      Description

      I just saw that block_navigation doesn't handle ampersands in course names correctly when creating the "My courses" list.

      We have a course with a ampersand in the course name. In the title of the course link in the mycourses list, the ampersand gets displayed as HTML Entity instead of being displayed as ampersand (see screenshot).

      As the content of the mycourses list comes from a AJAX call, I don't know where the issue is handled best.

        Activity

        Hide
        Sam Hemelryk added a comment -

        Thanks for the report Alexander.

        I've had a look at this now.
        The issue can be traced back to when a course is first added to the navigation at which point the fullname which is used for the title has been formatted and & have been changed to &.
        Having looked into the formatting functions and searching for existing use cases the best solution I could come up with was to deal with this issue at its source and replace & with & before setting the fullname as the title.

        Putting this up for peer-review now.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Thanks for the report Alexander. I've had a look at this now. The issue can be traced back to when a course is first added to the navigation at which point the fullname which is used for the title has been formatted and & have been changed to &. Having looked into the formatting functions and searching for existing use cases the best solution I could come up with was to deal with this issue at its source and replace & with & before setting the fullname as the title. Putting this up for peer-review now. Many thanks Sam
        Hide
        Adrian Greeve added a comment -

        Hi Sam,

        Your patch fixes the problem in the navigation block, but I also noticed that it was present in the My home area (I attached a screenshot). I had a bit more of a look around, but I didn't see any other areas that needed fixing. Perhaps you could also fix the 'My home' section at the same time.

        Thanks.

        Show
        Adrian Greeve added a comment - Hi Sam, Your patch fixes the problem in the navigation block, but I also noticed that it was present in the My home area (I attached a screenshot). I had a bit more of a look around, but I didn't see any other areas that needed fixing. Perhaps you could also fix the 'My home' section at the same time. Thanks.
        Hide
        Sam Hemelryk added a comment -

        Thanks Adrian,

        I've tided up the my courses page as well.

        Integrator - the patch for that differs on every branch unfortunately.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks Adrian, I've tided up the my courses page as well. Integrator - the patch for that differs on every branch unfortunately. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I'm a bit lost here... isn't the & way the correct one? When did that change?

        Show
        Eloy Lafuente (stronk7) added a comment - I'm a bit lost here... isn't the & way the correct one? When did that change?
        Hide
        Damyon Wiese added a comment - - edited
        <a title="A test &amp; title">A title</a>
        

        works fine when written in plain HTML. The issue is when the title is set with javascript

        branchicon.setAttribute('title', 'a test &amp; title'); 
        

        wont work - the JS is expecting plain text here not html.

        So - this could be fixed in the JS - but this works just as well and it's a small issue.

        Show
        Damyon Wiese added a comment - - edited <a title= "A test &amp; title" >A title</a> works fine when written in plain HTML. The issue is when the title is set with javascript branchicon.setAttribute('title', 'a test &amp; title'); wont work - the JS is expecting plain text here not html. So - this could be fixed in the JS - but this works just as well and it's a small issue.
        Hide
        Damyon Wiese added a comment -

        Thanks Sam,

        I tested this change on each branch and it works well.

        Pushed to 23, 24 and master.

        Show
        Damyon Wiese added a comment - Thanks Sam, I tested this change on each branch and it works well. Pushed to 23, 24 and master.
        Hide
        Frédéric Massart added a comment -

        While testing MDL-37983, I encountered the following exception:

        Debug info: SELECT id,category FROM {course} WHERE id = ?
        [array (
        0 => -3,
        )]
        Error code: invalidrecord
        Stack trace:
        line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown
        line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
        line 6569 of /lib/accesslib.php: call to moodle_database->get_record()
        line 93 of /blocks/course_overview/renderer.php: call to context_course::instance()
        line 86 of /blocks/course_overview/block_course_overview.php: call to block_course_overview_renderer->course_overview()
        line 284 of /blocks/moodleblock.class.php: call to block_course_overview->get_content()
        line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
        line 953 of /lib/blocklib.php: call to block_base->get_content_for_output()
        line 1005 of /lib/blocklib.php: call to block_manager->create_block_contents()
        line 315 of /lib/blocklib.php: call to block_manager->ensure_content_created()
        line 1217 of /lib/outputrenderers.php: call to block_manager->get_content_for_region()
        line 155 of /my/index.php: call to core_renderer->blocks_for_region()
        

        It appears that the recent change in this issue is causing this error. Apparently I have a remote course, and so no context associated to it.

        Show
        Frédéric Massart added a comment - While testing MDL-37983 , I encountered the following exception: Debug info: SELECT id,category FROM {course} WHERE id = ? [array ( 0 => -3, )] Error code: invalidrecord Stack trace: line 1376 of /lib/dml/moodle_database.php: dml_missing_record_exception thrown line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 6569 of /lib/accesslib.php: call to moodle_database->get_record() line 93 of /blocks/course_overview/renderer.php: call to context_course::instance() line 86 of /blocks/course_overview/block_course_overview.php: call to block_course_overview_renderer->course_overview() line 284 of /blocks/moodleblock.class.php: call to block_course_overview->get_content() line 232 of /blocks/moodleblock.class.php: call to block_base->formatted_contents() line 953 of /lib/blocklib.php: call to block_base->get_content_for_output() line 1005 of /lib/blocklib.php: call to block_manager->create_block_contents() line 315 of /lib/blocklib.php: call to block_manager->ensure_content_created() line 1217 of /lib/outputrenderers.php: call to block_manager->get_content_for_region() line 155 of /my/index.php: call to core_renderer->blocks_for_region() It appears that the recent change in this issue is causing this error. Apparently I have a remote course, and so no context associated to it.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I continue thinking this is totally wrong, sorry. Keeping apart JS...

        1) XHTML (XML) requires they all to be &amp; else, it's broken. 23_STABLE is XHTML, isn't it?
        2) HTML5 only allows simple & if followed by whitespace (basically), so "B&W" is wrong and "B & W" is ok (idiot ambiguous rules). But it supports &amp; always (exactly like XHTML).

        So IMO, the only correct assumption is to use &amp; everywhere in HTML. If JS requires those un-entitized... that's another problem.

        Show
        Eloy Lafuente (stronk7) added a comment - I continue thinking this is totally wrong, sorry. Keeping apart JS... 1) XHTML (XML) requires they all to be &amp; else, it's broken. 23_STABLE is XHTML, isn't it? 2) HTML5 only allows simple & if followed by whitespace (basically), so "B&W" is wrong and "B & W" is ok (idiot ambiguous rules). But it supports &amp; always (exactly like XHTML). So IMO, the only correct assumption is to use &amp; everywhere in HTML. If JS requires those un-entitized... that's another problem.
        Hide
        Damyon Wiese added a comment -

        Sounds like we should revert this and reopen - just waiting for confirmation from Sam.

        Show
        Damyon Wiese added a comment - Sounds like we should revert this and reopen - just waiting for confirmation from Sam.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just a side note, the initial ambiguous rule I commented @ 2) was changed by another, better, rule in the HTML5 spec, but still... single "&" are not allowed always, while &amp; are. I found this document, pretty good:

        http://mathiasbynens.be/notes/ambiguous-ampersands

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Just a side note, the initial ambiguous rule I commented @ 2) was changed by another, better, rule in the HTML5 spec, but still... single "&" are not allowed always, while &amp; are. I found this document, pretty good: http://mathiasbynens.be/notes/ambiguous-ampersands Ciao
        Hide
        Sam Hemelryk added a comment -

        Hmmm, my mind really isn't processing this at the moment. Certainly it needs more research. My vote goes to reverting the patches and going back to look at this more.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Hmmm, my mind really isn't processing this at the moment. Certainly it needs more research. My vote goes to reverting the patches and going back to look at this more. Many thanks Sam
        Hide
        Damyon Wiese added a comment -

        Thanks Sam,

        I have reverted this now so we can spend some more time on it.

        Show
        Damyon Wiese added a comment - Thanks Sam, I have reverted this now so we can spend some more time on it.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Sam Hemelryk added a comment -

        Ok after looking at this I think I've nutted it out.
        I've tidied up how the navigation sets the title attribute for a node, and have removed situations where we are double encoding through the use of combined format_string and html_writer attributes (which uses s()).

        Back up for peer-review.

        Show
        Sam Hemelryk added a comment - Ok after looking at this I think I've nutted it out. I've tidied up how the navigation sets the title attribute for a node, and have removed situations where we are double encoding through the use of combined format_string and html_writer attributes (which uses s()). Back up for peer-review.
        Hide
        Adrian Greeve added a comment -

        Hi Sam,

        I had a look at each of the different branches and I can see that you've brought the code in 2.3 and 2.4 more in line with the master branch.

        Just one thing. Perhaps you could alter the testing instructions to include checking that the title is correct on the 'My home' page.

        Thanks.

        Show
        Adrian Greeve added a comment - Hi Sam, I had a look at each of the different branches and I can see that you've brought the code in 2.3 and 2.4 more in line with the master branch. Just one thing. Perhaps you could alter the testing instructions to include checking that the title is correct on the 'My home' page. Thanks.
        Hide
        Sam Hemelryk added a comment -

        Thanks Adrian, putting this up for integration now.

        Show
        Sam Hemelryk added a comment - Thanks Adrian, putting this up for integration now.
        Hide
        Damyon Wiese added a comment -

        Thanks Sam,

        This addresses Eloys concerns about invalid html (uses & in attributes). I tested this on all branches and I also set remote enrolments on all branches to test the issue reported by Fred and found no issues.

        Integrated to master, 24 and 23.

        Show
        Damyon Wiese added a comment - Thanks Sam, This addresses Eloys concerns about invalid html (uses & in attributes). I tested this on all branches and I also set remote enrolments on all branches to test the issue reported by Fred and found no issues. Integrated to master, 24 and 23.
        Hide
        Frédéric Massart added a comment -

        The test is successful on master and 2.4, however there are a few problems with the title attribute on 2.3:

        • On my home page, as a student, the title is espaced too many times:
        <a title="Test &amp;amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">Test &amp; ampersands</a>
        
        // To me this title is irrelevant and should be removed, unless it's used when the content of the <a> is cropped.
        
        • In the navigation, under My courses, the title is escaped too many times:
        <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a>
        
        • In the navigation, under Courses, the title is escaped too many times:
        <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a>
        
        • In the breadcrumb, the title is escaped too many times:
        <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a>
        
        Show
        Frédéric Massart added a comment - The test is successful on master and 2.4, however there are a few problems with the title attribute on 2.3: On my home page, as a student, the title is espaced too many times: <a title="Test &amp;amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">Test &amp; ampersands</a> // To me this title is irrelevant and should be removed, unless it's used when the content of the <a> is cropped. In the navigation, under My courses , the title is escaped too many times: <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a> In the navigation, under Courses , the title is escaped too many times: <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a> In the breadcrumb, the title is escaped too many times: <a title="Test &amp;amp; ampersands" href="http://fred.moodle.local/i23/course/view.php?id=8">T&amp;A</a>
        Hide
        Frédéric Massart added a comment -

        My bad, 2.3 is fine as well. Thanks Damyon for pointing it out.

        Show
        Frédéric Massart added a comment - My bad, 2.3 is fine as well. Thanks Damyon for pointing it out.
        Hide
        Damyon Wiese added a comment -

        Passing for fred (see comment)

        Show
        Damyon Wiese added a comment - Passing for fred (see comment)
        Hide
        Dan Poltawski added a comment -

        Blooming Marvelous! It's time for a knees up - your changes are upstream!

        Thanks for making Moodle better!

        Toodle pip

        Show
        Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

          People

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

            Dates

            • Created:
              Updated:
              Resolved: