Moodle

We should extend phpmailer rather than customise in place

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.9.6, 2.0
  • Fix Version/s: 2.0
  • Component/s: Libraries
  • Labels:
    None

Description

Customising phpmailer in place causes great pain for us distros, but it also makes our changes to upstream hard to see. And makes upgrades of the library hard.

I just looked at our cutomisatisations to phpmailer and I am pretty sure we can do them all by extending the existing class ratehr than customising in place.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Dan Poltawski added a comment -

Some ntoes I just made

  • Adds message-id regardless if we set it ourselves - seems fixeable in new version
  • timezone http://tracker.moodle.org/browse/MDL-12596 - can textend
  • EncodeHeaer() - can extend that function
  • SetLanguage() - can etend that function
  • Constructor
Show
Dan Poltawski added a comment - Some ntoes I just made
  • Adds message-id regardless if we set it ourselves - seems fixeable in new version
  • timezone http://tracker.moodle.org/browse/MDL-12596 - can textend
  • EncodeHeaer() - can extend that function
  • SetLanguage() - can etend that function
  • Constructor
Hide
Permalink
Dan Poltawski added a comment -

As far as I can see, SetLanguage is not doing anything!

Show
Dan Poltawski added a comment - As far as I can see, SetLanguage is not doing anything!
Hide
Permalink
Dan Poltawski added a comment -

Here is a patch which does the job of all our customisations. With the exception of SetLanguage(), because I can't see this being used at all..

Show
Dan Poltawski added a comment - Here is a patch which does the job of all our customisations. With the exception of SetLanguage(), because I can't see this being used at all..
Hide
Permalink
Dan Poltawski added a comment -

(Note this depends on a recentish version fo phpmailer, rather than the one currently imported).

Show
Dan Poltawski added a comment - (Note this depends on a recentish version fo phpmailer, rather than the one currently imported).
Hide
Permalink
Dan Poltawski added a comment -

Eloy/Petr,

Please see my attached patch which moves our customisations from phpmailer into a subclass which just implements the functionality.

I have tested and believe this would mean that we would no longer need to customise phpmailer in place.

The only change I ddi not implement was that of SetLanguage() because I could not find any code paths which would call this.

What do you think (note it depends on recent versions of phpmailer)

Show
Dan Poltawski added a comment - Eloy/Petr, Please see my attached patch which moves our customisations from phpmailer into a subclass which just implements the functionality. I have tested and believe this would mean that we would no longer need to customise phpmailer in place. The only change I ddi not implement was that of SetLanguage() because I could not find any code paths which would call this. What do you think (note it depends on recent versions of phpmailer)
Hide
Permalink
Dan Poltawski added a comment -

(Targeting this at 2.0, and backporting to 1.9 for Debians purposes)

Show
Dan Poltawski added a comment - (Targeting this at 2.0, and backporting to 1.9 for Debians purposes)
Hide
Permalink
Petr Škoda (skodak) added a comment -

hmmm, I thought we are going to import latest PHP5 version into HEAD

Show
Petr Škoda (skodak) added a comment - hmmm, I thought we are going to import latest PHP5 version into HEAD
Hide
Permalink
Petr Škoda (skodak) added a comment -

I vaguely remember adding some ugly hacks into the mailer - looks like the reame lists a bit more things than the patch includes.

my -1 for any mailer changes in 1.9 unless it is a security problem
my +10 for using vanilla phpmailer in HEAD

Show
Petr Škoda (skodak) added a comment - I vaguely remember adding some ugly hacks into the mailer - looks like the reame lists a bit more things than the patch includes. my -1 for any mailer changes in 1.9 unless it is a security problem my +10 for using vanilla phpmailer in HEAD
Hide
Permalink
Dan Poltawski added a comment -

Petr - sorry my previous message was unclear. I am proposing what you suggest. No changes to 1.9.

(We will backport the change into debian only - where we can depend on php5 support etc)

Show
Dan Poltawski added a comment - Petr - sorry my previous message was unclear. I am proposing what you suggest. No changes to 1.9. (We will backport the change into debian only - where we can depend on php5 support etc)
Hide
Permalink
Dan Poltawski added a comment -

Re the readme:

  • Duplicate Message-IDs in Forum mail (MDL-3681)
    Fixed by AddCustomHeader() change in patch
  • Support for gb18030 (MDL-5229)
    Fixed by the EncodeHeader() changes
  • Correct timezone in date (MDL-12596)
    Fixed by replacing RFCDate (although we should re-report this upstream)
  • Backported fixes for CVE-2007-3215 (MDL-18348)
    Not relevant in new upstream version
  • Custom EncodeHeader() to allow multibyte subjects (textlib). Seems that current phpmailer version (2.3) has it properly implemented (though dependent of mbstring).
    Again changed by EncodeHeader() changed
  • Custom constructor PHPMailer()
    New constructor
  • Custom SetLanguage() function
    This is the only one I haven't changed, I can't see the code paths which this makes any difference?
Show
Dan Poltawski added a comment - Re the readme:
  • Duplicate Message-IDs in Forum mail (MDL-3681) Fixed by AddCustomHeader() change in patch
  • Support for gb18030 (MDL-5229) Fixed by the EncodeHeader() changes
  • Correct timezone in date (MDL-12596) Fixed by replacing RFCDate (although we should re-report this upstream)
  • Backported fixes for CVE-2007-3215 (MDL-18348) Not relevant in new upstream version
  • Custom EncodeHeader() to allow multibyte subjects (textlib). Seems that current phpmailer version (2.3) has it properly implemented (though dependent of mbstring). Again changed by EncodeHeader() changed
  • Custom constructor PHPMailer() New constructor
  • Custom SetLanguage() function This is the only one I haven't changed, I can't see the code paths which this makes any difference?
Hide
Permalink
Petr Škoda (skodak) added a comment -

now you are the one who understands this the most
I have to relearn everything every time I go and make changes there

Show
Petr Škoda (skodak) added a comment - now you are the one who understands this the most I have to relearn everything every time I go and make changes there
Hide
Permalink
Dan Poltawski added a comment -

I have committed and tested these changes.

Unfortunately forum emails seem to be broken in some way by the new messaging system at the moment so had to revert that to ensure forum stuff was working correctly.

If we could find a good way to create unit tests for this I think it would be very useful, but I can't really think of a good way to do this.

Show
Dan Poltawski added a comment - I have committed and tested these changes. Unfortunately forum emails seem to be broken in some way by the new messaging system at the moment so had to revert that to ensure forum stuff was working correctly. If we could find a good way to create unit tests for this I think it would be very useful, but I can't really think of a good way to do this.

People

Vote (2)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.