Replaced imap with mailpit and refreshed the test. #13219

SakiTakamachi · 2024-01-22T11:22:00Z

take2 of #13197

This uses mailpit

TODO: Once the SDKs are merged, change the SDK repository URL back

SakiTakamachi · 2024-01-22T17:05:39Z

@bukka

All comments received in the PR for the mailhog version have been corrected.

SakiTakamachi · 2024-01-22T17:12:59Z

I don't know if I should specify json as a required extension.

Girgias

LGTM

bukka

It looks good - just some minor comments

bukka · 2024-01-23T11:05:52Z

ext/standard/tests/mail/bug72964.phpt

    }
+
+    deleteEmail($res);


Is it enough to just clear a single email - wouldn't be better to just always clear all emails. That should be also added to CLEAN section in case of failure so nothing is left there.

I was honestly confused about this too.

Deleting all emails causes problems with parallel tests, but aren't these tests ever run in parallel?

Also, I was hesitant about the CLEAN section because I would have to use the API several times to delete just one email.

No, I forgot that unlike mailhog, mailpit can be easily deleted. I can use the CLEAN section.

Related to other comments, this is data for outgoing emails, so even if there are multiple destinations, there will only be one email.

bukka · 2024-01-23T11:08:58Z

ext/standard/tests/mail/bug80706.phpt

 } else {
-	echo "Message sent OK\n";
+    echo "Sent the email.\n";


NIT:

Suggested change

echo "Sent the email.\n";

echo "Email sent.\n";

and not much point to have else block for this...

bukka · 2024-01-23T11:12:54Z

ext/standard/tests/mail/bug80706.phpt

+$res = searchEmailByToAddress($to);
+
+if (mailCheckResponse($res, $from, $to, $subject, $message)) {
+    echo "Received the email.\n";


NIT:

Suggested change

echo "Received the email.\n";

echo "Email received.\n";

just for consistency with email sent ... :)

bukka · 2024-01-23T11:13:36Z

ext/standard/tests/mail/bug80706.phpt

+
+    $bccAddresses = getBccAddresses($res);
+    if (in_array($bcc, $bccAddresses, true)) {
+        echo "bcc Received the email.\n";


NIT: This doesn't exactly tests that BCC received the email but more that BCC is set, right?

Right. Maybe I should test that the email arrived correctly.

yeah and might make sense to do the same for CC. Btw. shouldn't BCC be excluded from the original delivered email?

Oops, I was careless.

mailpit (and mailhog) is like an SMTP dummy, so I may not be able to check the behavior on the receiving side.

This test checks "sent mail", not received mail.

Yeah but it's the sender who should send multiple emails. See https://stackoverflow.com/a/2750359 . So I would expect mailpit to have multiple emails received though. Might be worth to try to send BCC email with sendmail to see if it's behaving the same.

Yes but we need to verify that data is sent correctly. In this case it means that multiple RCPT TO are sent which was possible in the previous version. You can see that the client have special handling for BCC in that regard:

php-src/win32/sendmail.c

Lines 525 to 614 in 5da8335

/* Send mail to all Bcc rcpt's

This is basically a rip of the Cc code above.

Just don't forget to remove the Bcc: from the header afterwards. */

if (mailBcc && *mailBcc) {

tempMailTo = estrdup(mailBcc);

/* Send mail to all rcpt's */

token = find_address(tempMailTo, &token_state);

while (token != NULL)

{

SMTP_SKIP_SPACE(token);

FormatEmailAddress(PW32G(mail_buffer), token, "RCPT TO:<%s>\r\n");

if ((res = Post(PW32G(mail_buffer))) != SUCCESS) {

efree(tempMailTo);

return (res);

}

if ((res = Ack(&server_response)) != SUCCESS) {

SMTP_ERROR_RESPONSE(server_response);

efree(tempMailTo);

return (res);

}

token = find_address(NULL, &token_state);

}

efree(tempMailTo);

}

else if (headers) {

if ((pos1 = strstr(headers_lc, "bcc:")) && (pos1 == headers_lc || *(pos1-1) == '\n')) {

/* Real offset is memaddress from the original headers + difference of

* string found in the lowercase headers + 4 characters to jump over

* the bcc: */

pos1 = headers + (pos1 - headers_lc) + 4;

if (NULL == (pos2 = strstr(pos1, "\r\n"))) {

tempMailTo = estrndup(pos1, strlen(pos1));

/* Later, when we remove the Bcc: out of the

header we know it was the last thing. */

pos2 = pos1;

} else {

const char *pos3 = pos2;

while (pos2[2] == ' ' || pos2[2] == '\t') {

pos3 = strstr(pos2 + 2, "\r\n");

if (pos3 != NULL) {

pos2 = pos3;

} else {

pos2 += strlen(pos2);

break;

}

}

tempMailTo = estrndup(pos1, pos2 - pos1);

if (pos3 == NULL) {

/* Later, when we remove the Bcc: out of the

header we know it was the last thing. */

pos2 = pos1;

}

}

token = find_address(tempMailTo, &token_state);

while (token != NULL)

{

SMTP_SKIP_SPACE(token);

FormatEmailAddress(PW32G(mail_buffer), token, "RCPT TO:<%s>\r\n");

if ((res = Post(PW32G(mail_buffer))) != SUCCESS) {

efree(tempMailTo);

return (res);

}

if ((res = Ack(&server_response)) != SUCCESS) {

SMTP_ERROR_RESPONSE(server_response);

efree(tempMailTo);

return (res);

}

token = find_address(NULL, &token_state);

}

efree(tempMailTo);

/* Now that we've identified that we've a Bcc list,

remove it from the current header. */

stripped_header = ecalloc(1, strlen(headers));

/* headers = point to string start of header

pos1 = pointer IN headers where the Bcc starts

'4' = Length of the characters 'bcc:'

Because we've added +4 above for parsing the Emails

we've to subtract them here. */

memcpy(stripped_header, headers, pos1 - headers - 4);

if (pos1 != pos2) {

/* if pos1 != pos2 , pos2 points to the rest of the headers.

Since pos1 != pos2 if "\r\n" was found, we know those characters

are there and so we jump over them (else we would generate a new header

which would look like "\r\n\r\n". */

memcpy(stripped_header + (pos1 - headers - 4), pos2 + 2, strlen(pos2) - 2);

}

}

}

The client actually strips BCC but seems that mailpit probably adds it back axllent/mailpit@9c8329a

If it's the case, it seems to me that mailpit might be good more for testing the actual email messages rather than making sure that client works as expected. For that it's maybe better to keep the actual real server and possibly just only replace imap client to check it out.

That makes sense. It's already late today, so I'll try it tomorrow.

I have done some checking but only CLI clients seem to be like complete CLI apps (e.g. mutt and alpine) but didn't find anything that would not be probably ideal.

But then I found also this project: https://github.com/nodemailer/wildduck which offers API. I guess it can work just with SMTP mailserver but we already got so maybe that would be a better option. We would basically just replace usage of imap ext with calls to this service that could check the same thing potentially.

thank you. mailpit is a dummy SMTP server, so I may need something else to actually send.

If we want to test both sending and receiving behavior, things can get a little complicated.

I'll look into it a little more too.

In previous tests, the sending/receiving server was the hmail server, and the IMAP extension was acting as an IMAP/POP3 client to the hmail server. wildduck apparently primarily functions as a receiving server, so it needs to be able to receive emails for tests, which can get quite complicated. (In fact, if I look at the documentation, there will be instructions to configure DNS.)

I'll continue investigating.

github-actions bot added the Extension: standard label Jan 22, 2024

SakiTakamachi force-pushed the replace_imap_to_mailpit branch 6 times, most recently from 2e9bc34 to 382c355 Compare January 22, 2024 15:39

SakiTakamachi added 2 commits January 23, 2024 01:31

Replaced imap with mailpit and refreshed the test.

acfd100

wip

2d94d0f

SakiTakamachi force-pushed the replace_imap_to_mailpit branch from d8e25a3 to 2d94d0f Compare January 22, 2024 16:32

SakiTakamachi marked this pull request as ready for review January 22, 2024 17:03

SakiTakamachi requested review from bukka, iluuu1994 and TimWolla as code owners January 22, 2024 17:04

SakiTakamachi changed the title ~~[WIP] Replaced imap with mailpit and refreshed the test.~~ Replaced imap with mailpit and refreshed the test. Jan 22, 2024

SakiTakamachi requested review from Girgias and derickr January 22, 2024 17:09

iluuu1994 removed their request for review January 22, 2024 17:13

Girgias approved these changes Jan 23, 2024

View reviewed changes

Delete the unnecessary function

c437369

bukka reviewed Jan 23, 2024

View reviewed changes

Address review comments

7039ffa

SakiTakamachi force-pushed the replace_imap_to_mailpit branch from d722de1 to 7039ffa Compare January 23, 2024 12:55

SakiTakamachi mentioned this pull request Jan 26, 2024

Follow-up to remove IMAP ext #13248

Merged

SakiTakamachi closed this Jan 26, 2024

SakiTakamachi deleted the replace_imap_to_mailpit branch February 10, 2024 01:18

	/* Send mail to all Bcc rcpt's
	This is basically a rip of the Cc code above.
	Just don't forget to remove the Bcc: from the header afterwards. */
	if (mailBcc && *mailBcc) {
	tempMailTo = estrdup(mailBcc);
	/* Send mail to all rcpt's */
	token = find_address(tempMailTo, &token_state);
	while (token != NULL)
	{
	SMTP_SKIP_SPACE(token);
	FormatEmailAddress(PW32G(mail_buffer), token, "RCPT TO:<%s>\r\n");
	if ((res = Post(PW32G(mail_buffer))) != SUCCESS) {
	efree(tempMailTo);
	return (res);
	}
	if ((res = Ack(&server_response)) != SUCCESS) {
	SMTP_ERROR_RESPONSE(server_response);
	efree(tempMailTo);
	return (res);
	}
	token = find_address(NULL, &token_state);
	}
	efree(tempMailTo);
	}
	else if (headers) {
	if ((pos1 = strstr(headers_lc, "bcc:")) && (pos1 == headers_lc \|\| *(pos1-1) == '\n')) {
	/* Real offset is memaddress from the original headers + difference of
	* string found in the lowercase headers + 4 characters to jump over
	* the bcc: */
	pos1 = headers + (pos1 - headers_lc) + 4;
	if (NULL == (pos2 = strstr(pos1, "\r\n"))) {
	tempMailTo = estrndup(pos1, strlen(pos1));
	/* Later, when we remove the Bcc: out of the
	header we know it was the last thing. */
	pos2 = pos1;
	} else {
	const char *pos3 = pos2;
	while (pos2[2] == ' ' \|\| pos2[2] == '\t') {
	pos3 = strstr(pos2 + 2, "\r\n");
	if (pos3 != NULL) {
	pos2 = pos3;
	} else {
	pos2 += strlen(pos2);
	break;
	}
	}
	tempMailTo = estrndup(pos1, pos2 - pos1);
	if (pos3 == NULL) {
	/* Later, when we remove the Bcc: out of the
	header we know it was the last thing. */
	pos2 = pos1;
	}
	}

	token = find_address(tempMailTo, &token_state);
	while (token != NULL)
	{
	SMTP_SKIP_SPACE(token);
	FormatEmailAddress(PW32G(mail_buffer), token, "RCPT TO:<%s>\r\n");
	if ((res = Post(PW32G(mail_buffer))) != SUCCESS) {
	efree(tempMailTo);
	return (res);
	}
	if ((res = Ack(&server_response)) != SUCCESS) {
	SMTP_ERROR_RESPONSE(server_response);
	efree(tempMailTo);
	return (res);
	}
	token = find_address(NULL, &token_state);
	}
	efree(tempMailTo);

	/* Now that we've identified that we've a Bcc list,
	remove it from the current header. */
	stripped_header = ecalloc(1, strlen(headers));
	/* headers = point to string start of header
	pos1 = pointer IN headers where the Bcc starts
	'4' = Length of the characters 'bcc:'
	Because we've added +4 above for parsing the Emails
	we've to subtract them here. */
	memcpy(stripped_header, headers, pos1 - headers - 4);
	if (pos1 != pos2) {
	/* if pos1 != pos2 , pos2 points to the rest of the headers.
	Since pos1 != pos2 if "\r\n" was found, we know those characters
	are there and so we jump over them (else we would generate a new header
	which would look like "\r\n\r\n". */
	memcpy(stripped_header + (pos1 - headers - 4), pos2 + 2, strlen(pos2) - 2);
	}
	}
	}

Replaced imap with mailpit and refreshed the test. #13219

Replaced imap with mailpit and refreshed the test. #13219

Uh oh!

Conversation

SakiTakamachi commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Jan 22, 2024

Uh oh!

SakiTakamachi commented Jan 22, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SakiTakamachi commented Jan 22, 2024 •

edited

Loading