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:
    • Testing Instructions:
      Hide
      1. Add a new course with the title "Test & ampersands"
      2. Check in the navigation that the title for the course node is properly formatted.
      3. Enrol a student into the course and log in as that student.
      4. Visit the My home page and check that the title appears correctly.
      Show
      Add a new course with the title "Test & ampersands" Check in the navigation that the title for the course node is properly formatted. Enrol a student into the course and log in as that student. Visit the My home page and check that the title appears correctly.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-38055-m25

      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.

        Gliffy Diagrams

          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: