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

Fix quoted-printable encoding #82

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@danxexe

danxexe commented Sep 12, 2018

The QUOTED-PRINTABLE value for the Content-Transfer-Encoding header is stored as a underscored atom quoted_printable, but the encoder_for/1 function attempts to map by comparing this value to a dash separated string "quoted-printable". This results in the binary encoder being used by default, which breaks messages with = characters (see the changed test case). I kept the underscored mapping in order to keep backwards compatibility of the atom version of Mail.Message.put_header/3, which does not replace underscores with dashes.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Sep 13, 2018

Member

Would it not be better to just normalize on the hyphenation rather than carry both?

Member

bcardarella commented Sep 13, 2018

Would it not be better to just normalize on the hyphenation rather than carry both?

@danxexe

This comment has been minimized.

Show comment
Hide comment
@danxexe

danxexe Sep 13, 2018

We could and it would probably be more consistent, but as I said, it would break compatibility for people that rely on the current default (:quoted_printable) set by Mail.Message.build_text/1 and Mail.Message.build_html/1.

danxexe commented Sep 13, 2018

We could and it would probably be more consistent, but as I said, it would break compatibility for people that rely on the current default (:quoted_printable) set by Mail.Message.build_text/1 and Mail.Message.build_html/1.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Sep 18, 2018

Member

Instead of adding the second encoding I would add a new normalize/1 function replacing this LOC:

|> Atom.to_string()

This function, for the sake of performance, for now can simply be:

defp normalize(:quoted_printable), do: normalize(:"quoted-printable")
defp normalize(encoding) when is_atom(encoding), do: Atom.to_string(encoding)
defp normalize(encoding) when is_binary(encoding), do: encoding
Member

bcardarella commented Sep 18, 2018

Instead of adding the second encoding I would add a new normalize/1 function replacing this LOC:

|> Atom.to_string()

This function, for the sake of performance, for now can simply be:

defp normalize(:quoted_printable), do: normalize(:"quoted-printable")
defp normalize(encoding) when is_atom(encoding), do: Atom.to_string(encoding)
defp normalize(encoding) when is_binary(encoding), do: encoding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment