Replace deprecated Network usage #25
Conversation
Network/Mail/SMTP.hs
Outdated
| @@ -137,8 +137,14 @@ parseResponse conn = do | |||
| (code, bdy) <- readLines | |||
| return (read $ B8.unpack code, B8.unlines bdy) | |||
| where | |||
| getLines c acc = do | |||
There was a problem hiding this comment.
@parsonsmatt I need some help with this as I am not sure if this is right or should I just stick with connectionGetLine. Having to specify max bytes per line seems a little off to me.
There was a problem hiding this comment.
thinking out loud here:
- This function is replacing
B8.hGetLine :: Handle -> IO ByteString. Implementation linked. - This function reads from the bytestring until it either finds a
\ncharacter or the end-of-file. (figuring this out exactly took me a long time, dang the bytestring internals are tough) connectionGetLinedoesn't work in exactly the same way. TheIntparameter is to prevent DoS attacks, so it'd be a security improvement to have it for sure - no idea what typical line length limit might be safe here 🤷♂️ .- The bigger problem with
connectionGetLineis that it throwsisEOFErrorexception on end of input. Except, it says "An end of file will be considered as a line terminator too, if line is not empty." - Looking at the source... well, it's a bit confusing, but I think I've worked out what is going on.
I think we're fine with connectionGetLine, we just need to pick a suitable line length.
There was a problem hiding this comment.
I did a quick Google and got this StackOverflow post re line lengths in mail.
I think 1000 is a reasonable pick. The standard says lines MUST be no more than 998 characters.
| @@ -309,27 +318,19 @@ simpleMail from to cc bcc subject parts = | |||
|
|
|||
| -- | Construct a plain text 'Part' | |||
| plainTextPart :: TL.Text -> Part | |||
| plainTextPart = Part "text/plain; charset=utf-8" | |||
| QuotedPrintableText Nothing [] . TL.encodeUtf8 | |||
| plainTextPart body = Part "text/plain; charset=utf-8" | |||
There was a problem hiding this comment.
@parsonsmatt mime-mail has plainPart, htmlPart, and filePart. Should I just remove these and prefer ones from mime-mail instead?
There was a problem hiding this comment.
This PR is a breaking change to the type of exposed functions, and thus a major version bump. I think it'd be fine to write DEPRECATED pragmas for these functions, point users to the relevant mime-mail replacements, and we'll remove in another major version bump.
|
@parsonsmatt What do you think about these changes? Are there anything you like me to change in order to get this PR in? |
| @@ -309,27 +318,19 @@ simpleMail from to cc bcc subject parts = | |||
|
|
|||
| -- | Construct a plain text 'Part' | |||
| plainTextPart :: TL.Text -> Part | |||
| plainTextPart = Part "text/plain; charset=utf-8" | |||
| QuotedPrintableText Nothing [] . TL.encodeUtf8 | |||
| plainTextPart body = Part "text/plain; charset=utf-8" | |||
There was a problem hiding this comment.
This PR is a breaking change to the type of exposed functions, and thus a major version bump. I think it'd be fine to write DEPRECATED pragmas for these functions, point users to the relevant mime-mail replacements, and we'll remove in another major version bump.
Network/Mail/SMTP.hs
Outdated
| @@ -137,8 +137,14 @@ parseResponse conn = do | |||
| (code, bdy) <- readLines | |||
| return (read $ B8.unpack code, B8.unlines bdy) | |||
| where | |||
| getLines c acc = do | |||
There was a problem hiding this comment.
thinking out loud here:
- This function is replacing
B8.hGetLine :: Handle -> IO ByteString. Implementation linked. - This function reads from the bytestring until it either finds a
\ncharacter or the end-of-file. (figuring this out exactly took me a long time, dang the bytestring internals are tough) connectionGetLinedoesn't work in exactly the same way. TheIntparameter is to prevent DoS attacks, so it'd be a security improvement to have it for sure - no idea what typical line length limit might be safe here 🤷♂️ .- The bigger problem with
connectionGetLineis that it throwsisEOFErrorexception on end of input. Except, it says "An end of file will be considered as a line terminator too, if line is not empty." - Looking at the source... well, it's a bit confusing, but I think I've worked out what is going on.
I think we're fine with connectionGetLine, we just need to pick a suitable line length.
Network/Mail/SMTP.hs
Outdated
| res <- Conn.connectionGetChunk c | ||
| if B8.null res | ||
| then return acc | ||
| else getLines c $ B8.append acc res |
There was a problem hiding this comment.
This current implementation does not return a line, but instead returns the entire contents of the connection. Replacing it with connectionGetLine will fix it.
Network/Mail/SMTP.hs
Outdated
| @@ -137,8 +137,14 @@ parseResponse conn = do | |||
| (code, bdy) <- readLines | |||
| return (read $ B8.unpack code, B8.unlines bdy) | |||
| where | |||
| getLines c acc = do | |||
There was a problem hiding this comment.
I did a quick Google and got this StackOverflow post re line lengths in mail.
I think 1000 is a reasonable pick. The standard says lines MUST be no more than 998 characters.
|
@parsonsmatt I resolved all feedbacks. Let's get it merge if you think it's okay now 😃 |
This PR removes the use of deprecated
Networkmodule (fromnetwork). Unfortunately there are breaking changes but they are pretty easy to fix (only change of types).It is still work in progress but I like to get an early feedback if this is the way going forward. I am using this at work, so preferably we like to get this merged at some point when this PR is ready (should be soon).