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

in_reply_to treated as an iterable object, but its not #26

Closed
DasTobbel opened this issue Oct 13, 2020 · 8 comments
Closed

in_reply_to treated as an iterable object, but its not #26

DasTobbel opened this issue Oct 13, 2020 · 8 comments
Labels
bug Something isn't working enhancement New feature or request validated

Comments

@DasTobbel
Copy link
Contributor

Describe the bug
Kinda two bugs in one:

  1. In src/Header.php:475 the Header::parseAddresses($list) function requires the given $list to be an iterable object, in the case of the "in_reply_to" header, this will only happen if there was an ongoing discussion based on one mail.
    The more common case (probably?) is that it references the one mail on which the current mail responds to.

  2. In src/Header.php:460 the Header::extractAddresses($header) function wants to parse the in_reply_to header key as an email address, but it should not be a valid email address per RFC, it should be a message identifier, see :
    https://tools.ietf.org/html/rfc2822
    3.6.4. Identification fields

[...]
The "In-Reply-To:" field will contain the contents of the "Message-
   ID:" field of the message to which this one is a reply (the "parent
   message").  If there is more than one parent message, then the "In-
   Reply-To:" field will contain the contents of all of the parents'
   "Message-ID:" fields.  If there is no "Message-ID:" field in any of
   the parent messages, then the new message will have no "In-Reply-To:"
   field.
[...]

Dont know if still want to parse it to an array with some selectable fields, but should work if the bug 1 is solved.

To Reproduce
have a answered mail in your inbox, try to recieve. The Mail needs to be the first answer
Expected behavior
A clear and concise description of what you expected to happen.

Desktop / Server (please complete the following information):

  • OS: ubuntu 20
  • PHP: 7.4
  • Version 2.1.11
@DasTobbel
Copy link
Contributor Author

One possible solution is :

    private function parseAddresses($list) {
        $addresses = [];
        $list = ( is_array( $list ) || $list instanceof Traversable ) ? $list : [$list];
        foreach ($list as $item) {
        [...]

but i somehow don't like the fact were just assuming it should be an parseable email address then ....

@Webklex
Copy link
Owner

Webklex commented Oct 13, 2020

Hi @DasTobbel ,
thanks for your report. I think the best would be to remove in_reply_to from the array in src/Header.php:461 in order to be rfc compliant?

Best regards,

@Webklex Webklex added bug Something isn't working validated labels Oct 13, 2020
@DasTobbel
Copy link
Contributor Author

I think it would be the easiest and most compliant way for the stated issue.

Aftermath:
Whats about the setter "setInReplyTo(array)"?

src/Message.php:77 
 * @method array setInReplyTo(array $in_reply_to)

in RFC 2822 it should always be set as exactly one message identifier:
Page 23 -> in-reply-to = "In-Reply-To:" 1*msg-id CRLF

But RFC 4021 says:
Page 10-> Description: Identify replied-to message(s)

@Webklex
Copy link
Owner

Webklex commented Oct 13, 2020

So the correct way would be to support both and remove the "address" parsing part since it isn't required at all.
..but this could lead to confusion - so perhaps fetch / parse both (rfc2822 and rfc4021) and treat in-reply-to always as an array?

@DasTobbel
Copy link
Contributor Author

Did a bit of reading and research in my mails:

  1. RFC822 - ARPA Internet Messaging - defines the in-reply-to as "ids of previous correspondence", sounds like >= 1 to me

4.6.2. IN-REPLY-TO

         The contents of this field identify  previous  correspon-
    dence  which this message answers.  Note that if message iden-
    tifiers are used in this  field,  they  must  use  the  msg-id
    specification format.
  1. RFC 5322 - - Page 26, third paragraph

The "In-Reply-To:" field will contain the contents of the
"Message-ID:" field of the message to which this one is a reply (the
"parent message"). If there is more than one parent message, then
the "In-Reply-To:" field will contain the contents of all of the
parents' "Message-ID:" fields. If there is no "Message-ID:" field in
any of the parent messages, then the new message will have no "In-
Reply-To:" field.

  1. i did not have any mail on my side with more then one in-reply-to message id
  2. somewhat similiar question on stack overflow, but sadly only 1 response... Suggests that there is the possibility that there is more then one , but most likely will not...
  3. this stack overflow thread suggest that microsoft supports only 1 in-reply-to
  4. And as we all know, Wikipedia tells the truth
    In-Reply-To: Message-ID of the message this is a reply to.

I conclude, to be fully "rfc" there should be a setter and getter for a single message id and for an array of those...
Pull request for:

// $in_reply_to | single message id format (<messageid@host>) or array of message ids
setInReplyTo( $in_reply_to ) {
    if ( is_array($in_reply_to ) ) {
        $this->in_reply_to = implode(', ', $in_reply_to);
    } else {
       $this->in_reply_to = $in_reply_to;
    }
}
getInReplyTo(){
    if ( strpos( $this->in_reply_to, ',') !== false) {
        return explode(',' ,$this->in_reply_to);
    } else {
        return $this->in_reply_to:
    }
}
?

@Webklex Webklex added the enhancement New feature or request label Oct 14, 2020
@Webklex
Copy link
Owner

Webklex commented Oct 14, 2020

@DasTobbel thanks for all the time spend on the research. I'm all in one the setter method 👍 But I kind of don't like the mixed response in getInReplyTo(). I think it would be better to either always return an array or a string. Or introduce a new config option to set the desired output format. Like options.fetch_in_reply_to (string, array, dynamic) ?

Best regards,

@Webklex
Copy link
Owner

Webklex commented Oct 15, 2020

Hi @DasTobbel ,
what do you think about adding:

if (property_exists($header, 'in_reply_to')) {
    $this->attributes["in_reply_to"] = is_array($header->in_reply_to) ? $header->in_reply_to : [$header->in_reply_to];
}

in Header::parse() and remove in_reply_to from the array in Header:L461.
That way in_reply_to supports all rfc standards and the user can be sure to always get an array of any existing in_reply_to headers.

@DasTobbel
Copy link
Contributor Author

I like that! As you said its nice for us users that we can rely on the responses - and we don't need to evaluate how to handle the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request validated
Projects
None yet
Development

No branches or pull requests

2 participants