[Catalyst-dev] Catalyst plugin

[J. Shirley]

On Tue, Sep 30, 2008 at 10:35 AM, Octavian Rasnita <orasnita at gmail.com> wrote:
> Hi,
>
> From: "J. Shirley" <jshirley at gmail.com>
>>>
>>> You should see about patching Catalyst::View::Email instead, I think.
>>>
>>
>> Yes, definitely.  It's admirable to want to contribute to open source,
>> CPAN, etc.  It is less noble to think the only way to do this is to
>> step on the toes of others and reinvent wheels.  I've always been very
>> open to patches on C::V::Email, and since email is a very big part the
>> more eyes and hands on it the better.
>>
>> If you just go off and write your own, you'll likely miss several key
>> things, or just outright fork the code and nobody wins.  If there is
>> something you distinct that you don't like about it (I understand some
>> people don't like the setup in stash, but that can be done away with
>> via simple methods that abstract that away) then discuss that and see
>> if we can come up with a solution.
>
> I also don't like the thing that we must use the stash very much, but I can
> live with it.
> What I am searching for is a module that allows me to send UTF-8 encoded
> emails (body and headers) without needing to specify MIME types,
> Content-Type, without using the Encode module, and of course allow me to add
> attachments and send it with a text and an html part.
> That's why I think Mail::Builder is better because it can do these things
> easy.
>
> I can try to change the way C::V::Email creates the message if you think
> this is OK, but if you think it is not, then I think I need to create
> another module.
>
> I also think the interface should allow us using something like:
>

Lets not work on changing the interface and instead focus on adding
attachment support :)

When sending email, you should always specify content type of the
parts though.  If you like the htmltext aspects of Mail::Builder,
there are other ways (convenience methods, etc), but they would
possibly require further upstream changes with
Catalyst::View::Email::Template (which uses TT to generate HTML) --
and I don't want to automatically assume that TT => HTML, but I'm fine
with a simple configuration switch, but if your only complaint is that
you don't want to set your specific content type, I think it is best
to shelve that until after the attachment work.

> $c->stash->{email} = {
[ snip, I don't want to discuss interface changes right now, lets just
focus on attachments]
>  attachment => 'file_name', #or:
>  attachment => ['file_name1', 'file_name2'],
>  image => 'image.png', #for displaying an inline image
>  mailer_args => ['SMTP', Host => 'mail.host.com'],
> };

Images aren't the only thing that can be embedded, and the inline
aspect can be a bit tricky to get right.  This is why
Email::MIME::CreateHTML looks the most advantageous for handling
embedding media.

If we drop that case, and then just focus on handling:
$c->stash->{email}->{attachment} = [ 'file1', 'file2' ]; # (or just a
simple scalar)

>
> If we would use Mail::Builder, it would be very simple to add attachments
> because we would just need to add
>
> attachment => 'file_name'
> or
> attachment => [$filename1, $filename2, $filename3],
>

Right, but that doesn't answer embedded images.  Adding an attachment
is very simple.  It is just another part that goes into Email::MIME.

If you just chuck an attachment in the parts, it should DTRT and send
the attachment.  In that regard, Catalyst::View::Email already does
handle attachments.  The only step is to have a simpler syntax and
handle embedded images.

Here's an example that should work:

$c->stash->{email}->{parts} = [
      Email::MIME->create(
          attributes => {
              filename     => "report.pdf",
              content_type => "application/pdf",
              encoding     => "quoted-printable",
              name         => "2004-financials.pdf",
          },
          body => io( "2004-financials.pdf" )->all,
      ),
];

The problem with your syntax above is that it doesn't handle the
content_type (we can infer, so no big deal), the encoding (again, we
can infer), the filename and name aspects shouldn't be tightly
married, either.

This is the issue with Email by and large.  The syntax is either too
simple for most cases, or too complex for most cases.

Having something like $c->stash->{email}->{attachments} = [
'filename.pdf' ] is fine with me, assuming that the patch is just
something along these lines:

my @parts = @{ $stash->{parts} };
foreach my $attachment ( @{$stash->{attachments}} ) {
   push @parts, Email::MIME->create(
          attributes => {
              filename     => $attachment,
              content_type => infer_mimetype( $attachment ),
              encoding     => "quoted-printable",
              name         => $attachment,
          },
          body => io( $c->path_to($attachment) )->all,
      ),
}


> I haven't tried it, but I am almost sure it doesn't handle UTF-8 encoded
> headers well, so if I would use the following code, the message won't be
> well formatted:
>
> $c->stash->{email} = {
> #...
> subject => 'To Octavian Râsnita',
> };
>

That's a separate issue then.  Try first, if it fails, submit an RT
request.  The patch may be to move to Mail::Builder, but Mail::Builder
is just yet another wrapper on top of MIME::Tools, whereas Email::Send
is on the Email::* modules.  I have more familiarity with the Email::*
modules and that makes me more comfortable.  If you want to switch to
Mail::Builder, you'll need to provide ample reasoning and prove that
there is valid reasons to do so.  I don't want to migrate away from
PEP without just cause.

> The advantage of Mail::Builder is that it automaticly encodes to UTF-8 the
> headers and the mail body, and one more thing... it gets easier the mail
> fields if they contain only a scalar (such an email address), or an arrayref
> (such a [$email, $name]).
>

Actually, it encodes subject and organization to MIME-Header RFC 2047.
 This would be a reasonable -separate- patch to C::V::Email to
encode('MIME-Header', $subject), as per Mail::Builder (I didn't check
if Email::Stuff did this, but I would assume so because it seems a
more PEP-friendly version of Mail::Builder).

I don't see where it is automatically encoding the body, if you can
point out in the Mail::Builder source I'd appreciate it.  It's
automatic conversion from htmltext to plaintext is also not desirable
to me, but perhaps others would like it -- this is a call for
comments.

> Then the created message could be sent directly with Email::Send and this
> has the advantage of beeing able to use more mailers.
>

Catalyst::View::Email already uses Email::Send.  Hence, all the
mailers.  Catalyst::View::Email is a wrapper on top of Email::*
modules, most notably Email::Send and Email::MIME.

If you're serious about participating, I would recommend you RTFS
Catalyst::View::Email, Mail::Builder and Email::Stuff.


> Ok, what's next? What should I do to start improving C::V::Email?
> Do you agree with my suggestions?

You have 3 different things going on here.  Lets break them down.

#1 is the attachments
#2 is moving to a higher level Email::Stuff or Mail::Builder setup,
which should be very seriously considered.
#3 is your API changes, which I don't even want to get into until #1
and #2 are done.

> I think they are improvements and they won't affect new users, but the
> problem is that the old code that probably uses other modules for creating
> the messages body might not work well.

I'm strongly resistant to API changes without a lot of merit to them
(things like "more_to" or having default sender status in config will
likely never get by me.)  Just a heads up.

-J