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

          Attachments

            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