Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for object representation of the header value #35

Closed
wants to merge 3 commits into from

Conversation

pali
Copy link
Contributor

@pali pali commented Jan 14, 2017

Allow to pass objects with as_mime_string() method into header_set() and create() methods. Object method as_mime_string() is responsible for encoding object into MIME string.

Add new method header_as_obj() which returns an object representation of the header value. If not explicitly specified object class is taken from the hash: %Email::MIME::Header::header_to_class_map.

…header_set() and create() methods

Object method as_mime_string() is responsible for encoding object into MIME string.
…epresentation of the header value

If not explicitly specified object class is taken from the hash:
%Email::MIME::Header::header_to_class_map
Copy link
Owner

@rjbs rjbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, apart from two small things. We can get this into a dev release as soon as we discuss those.

new object via method C<from_mime_string> of specified class. Input argument
for that class method is list of the raw MIME-encoded values. If class argument
is not specified then class name is taken from the hash
C<%Email::MIME::Header::header_to_class_map> via key field.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer that this come from a hashref returned by a method. It can still be updated by users, because it will return the same hashref each time, but to have your own set of behaviors, you just subclass Email::MIME and replace that method. This, I think, will end up being a lot cleaner than localizing the hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatives are:

  • Introduce functions like: register_class_map(header, class) and unregister_class_map(header, class) (which will directly manipulate with that hash)
  • Introduce function header_to_class_map() which will return reference to hash and user can directly modify it (or another class reimplement)
  • Introduce our hash variable %header_to_class_map and user can directly use it

Basically all those alternatives provides needed API for own headers, just at different level of abstraction. I have no strong preference between for any of those. So if you think that second option (function which returns reference to that hash) is the best I will change my patches to it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not care for the third option you present. The second is what I suggested. The first is (effectively) sugar on the second, as the two methods must share storage, and the simplest thing is for them to be implemented in terms of the second. They needn't be, of course.

The benefit of the first is that the registration method can detect and complain that someone is trying to change something already registered, which can prevent conflict at a distance by requiring anybody who wants to change a registration to first clear any existing one.

The benefit of the third is that one can localize the entries in the hash.

You can also localize the entries in the hash returned by a class method:

{
  my $map = Email::MIME->header_to_class_map;
  local $map->{'Message-Id'} = 'Some::Class';
  do_work;
}

...but this makes it clear that direct hash access is no good. People will get the casing wrong and that will stink. There should be a method to register and unregister, and they should case-flatten. They should be called set_class_for_header and clear_class_for_header.

Later, we could let them be set on a per-header-object basis, maybe, if that becomes useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. So instead of exporting our %header_to_class_map I will create new function set_class_for_header which modify internal hash map. So basically only first option. Are you fine with it? If clear_class_for_header is really needed we can add it later.

@@ -16,6 +17,10 @@ sub maybe_mime_encode_header {
$header = lc $header;

my $header_length = length($header) + length(": ");

return $val->as_mime_string($charset, $header_length)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A look suggests that nothing should be passing anything to maybe_mime_encode_header other than the value UTF-8. If this is the case, I think we should eliminate the argument. Header objects that know how to encode their strings should be free to pick the appropriate encoding (which imho should either be ASCII or UTF-8 and nothing else), and shouldn't have to deal with someone asking for emoji to be encoded as KOI-8.

Any reason to keep this argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see email clients or email services which do not support UTF-8, only ISO-8859-1. IIRC RFC 2047 does not require implementation to really support UTF-8, but require some ISO-8859-1 encoding.

As we know that MIME encoder and decoder in core perl (via Encode package) was terrible broken for a long time I would rather have needed functions for other encodings available. Just in case somebody needs to deal with other encoding as UTF-8 (e.g. that ISO-8859-1).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm sold. Let's reverse the argument order so that it's easier to give the header length and let the encoder pick an encoding, if the client doesn't care?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably $header and $header_length are misleading here. $header is field name (according to RFC2822) and $header_length length of field name + 2 (for colon and space).

Purpose of passing $header_length into encoder is ability to know how many octets are already print on first line before field body. Encoder can use this information and optimize whole header to fil into less lines. But it is just optional... Email::Simple can wrap correctly field body produced by encoder...

Parameter $header_length is optional for encoder, it does not have to use it. So I put it as second argument.

On the other hand, if $charset is passed, then encoder should use it and encode field body into that charset. Therefore I think $charset is more "required" as $header_length so I put $charset before $header_length.

If you have other opinion or better idea for naming variables just propose it...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. I'm not 💯 on either ordering, so I'll go with what you have and hope that nobody uses either one of those arguments. :-)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the code to use named arguments and avoid the whole question of order.

It will safely without overwriting modify hash: %Email::MIME::Header::header_to_class_map
@rjbs
Copy link
Owner

rjbs commented Jun 9, 2017

I have uploaded Email-MIME-1.943-TRIAL

@rjbs
Copy link
Owner

rjbs commented Jul 25, 2017

This branch is now landed and released in stable.

@rjbs rjbs closed this Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants