R18632 - rt/3.8/trunk/lib/RT/Action

RT Developers
1

I’m concerned that this change makes the regular expression too tight.
Previously, any value would work.

I know I’ve used values like “on”, “true” and “y” in the patch.

how about instead matching on no|0|off?On Tue, Mar 03, 2009 at 07:42:16AM -0500, elacour@bestpractical.com wrote:

Author: elacour
Date: Tue Mar 3 07:42:14 2009
New Revision: 18632

Modified:
rt/3.8/trunk/lib/RT/Action/SendEmail.pm

Log:
Take care of RT-Attach-Message value so we don’t add attachments if the value
is “no” for example (closes: #13259).
Thanks to Paul Vlaar.

Modified: rt/3.8/trunk/lib/RT/Action/SendEmail.pm

— rt/3.8/trunk/lib/RT/Action/SendEmail.pm (original)
+++ rt/3.8/trunk/lib/RT/Action/SendEmail.pm Tue Mar 3 07:42:14 2009
@@ -217,7 +217,8 @@
‘mime_words_ok’, );

 # Build up a MIME::Entity that looks like the original message.
  • $self->AddAttachments if $MIMEObj->head->get(‘RT-Attach-Message’);

  • $self->AddAttachments if ( $MIMEObj->head->get(‘RT-Attach-Message’)

  •                           && ( $MIMEObj->head->get('RT-Attach-Message') =~ /^yes$/i ) );

    $self->AddTickets;

Rt-commit mailing list
Rt-commit@lists.bestpractical.com
rt-commit Info Page

2

I’m concerned that this change makes the regular expression too tight.
Previously, any value would work.

I know I’ve used values like “on”, “true” and “y” in the patch.

how about instead matching on no|0|off?

well, that’s not common to do such thinks, think that maybe in future we
can find other values for this header to do other message attachement
related thinks.
but if you think that maching for y/yes/on/true can break existing
configs, I can do that.

3

I know I’ve used values like “on”, “true” and “y” in the patch.

how about instead matching on no|0|off?

well, that’s not common to do such thinks, think that maybe in future we
can find other values for this header to do other message attachement
related thinks.
but if you think that maching for y/yes/on/true can break existing
configs, I can do that.

Basically, when adding a new feature to deal with a previously uncovered
case, the conservative thing to do is to explicitly and carefully match
on your new case, rather than make guesses about how people might be
using the existing feature in the wild. By matching tightly on ‘^yes$’,
you’ve effectively changed the default. And changing defaults out from
under people is bad.

-Jesse

4

I know I’ve used values like “on”, “true” and “y” in the patch.

how about instead matching on no|0|off?

well, that’s not common to do such thinks, think that maybe in future we
can find other values for this header to do other message attachement
related thinks.
but if you think that maching for y/yes/on/true can break existing
configs, I can do that.

Basically, when adding a new feature to deal with a previously uncovered
case, the conservative thing to do is to explicitly and carefully match
on your new case, rather than make guesses about how people might be
using the existing feature in the wild. By matching tightly on ‘^yes$’,
you’ve effectively changed the default. And changing defaults out from
under people is bad.

Ok I commited a fix in this way.

5

I know I’ve used values like “on”, “true” and “y” in the patch.

how about instead matching on no|0|off?

well, that’s not common to do such thinks, think that maybe in future we
can find other values for this header to do other message attachement
related thinks.
but if you think that maching for y/yes/on/true can break existing
configs, I can do that.

Basically, when adding a new feature to deal with a previously uncovered
case, the conservative thing to do is to explicitly and carefully match
on your new case, rather than make guesses about how people might be
using the existing feature in the wild. By matching tightly on ‘^yes$’,
you’ve effectively changed the default. And changing defaults out from
under people is bad.

Ok I commited a fix in this way.

Thanks!