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

Forum email subjects containing ampersand are sent as &

    Details

    • Testing Instructions:
      Hide
      1. Set $CFG->divertallemailsto = 'youremailaddress'; in config.php
      2. As admin..
      3. Create a course and name its shortname "A&B"
      4. Enrol a student
      5. Add a news forum
      6. Create a new post titled "Test & Test" with a message
      7. Tick the mail now button and post to forum
      8. hit admin/cron.php (hopefully there will be a message about forum mailed out)
      9. Check your inbox and look at the title of the email
      Show
      Set $CFG->divertallemailsto = 'youremailaddress'; in config.php As admin.. Create a course and name its shortname "A&B" Enrol a student Add a news forum Create a new post titled "Test & Test" with a message Tick the mail now button and post to forum hit admin/cron.php (hopefully there will be a message about forum mailed out) Check your inbox and look at the title of the email
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      Forum email subject lines for topics with ampersand (&) in them are being replaced with &

      Other special characters do not seem to be affected. It appears format_string is responsible for this mangling:

      $string = replace_ampersands_not_followed_by_entity($string);

      Patch incoming that adds a simple html_entity_decode after this step, so the email subject line is returned to normal.

        Gliffy Diagrams

          Activity

          Show
          tlevi Tony Levi added a comment - https://github.com/tlevi/moodle/tree/mdl31458
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for the patch Tony. Unfortunately I don't think it solved the case of a course short-name with an ampersand and so I have made a modified patch using html_to_text on the whole subject.

          To be honest its a bit annoying that we need to format_text it at all since we are encoding text only to then decode it which is a waste of cpu cycles..

          However I believe we need to do it for multilang titles so can't get rid of the formatig completely.

          Submitting for integration.

          Show
          poltawski Dan Poltawski added a comment - Thanks for the patch Tony. Unfortunately I don't think it solved the case of a course short-name with an ampersand and so I have made a modified patch using html_to_text on the whole subject. To be honest its a bit annoying that we need to format_text it at all since we are encoding text only to then decode it which is a waste of cpu cycles.. However I believe we need to do it for multilang titles so can't get rid of the formatig completely. Submitting for integration.
          Hide
          tlevi Tony Levi added a comment -

          Fair enough - IIRC there were a few reported cases this patch didn't catch.

          Show
          tlevi Tony Levi added a comment - Fair enough - IIRC there were a few reported cases this patch didn't catch.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Dan, this has been integrated now

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Dan, this has been integrated now
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Working as expected.
          Title is properly displayed now.
          Tested on 21, 22 and master
          Passing
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Working as expected. Title is properly displayed now. Tested on 21, 22 and master Passing Thanks
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                9/Jul/12