Add support for object representation of the header value#35
Add support for object representation of the header value#35pali wants to merge 3 commits intorjbs:masterfrom
Conversation
…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
rjbs
left a comment
There was a problem hiding this comment.
This looks good to me, apart from two small things. We can get this into a dev release as soon as we discuss those.
lib/Email/MIME.pm
Outdated
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alternatives are:
- Introduce functions like:
register_class_map(header, class)andunregister_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_mapand 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| my $header_length = length($header) + length(": "); | ||
|
|
||
| return $val->as_mime_string($charset, $header_length) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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
|
I have uploaded Email-MIME-1.943-TRIAL |
|
This branch is now landed and released in stable. |
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.