Replaced imap with mailhog and refreshed the test. #13197

SakiTakamachi · 2024-01-19T16:13:50Z

The original test wasn't very good, so I improved it by the way.

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

SakiTakamachi · 2024-01-21T12:19:31Z

I replaced imap with mailhog and refreshed the test.

Since the SDK has not been merged yet, it is temporarily downloading the SDK from my repository.

php/php-sdk-binary-tools#8

Girgias

Looks fine? But I don't really understand email in the first place.

The only reason I was the maintainer for ext/imap was because I did the migration of resource to object

Girgias · 2024-01-22T01:07:43Z

ext/standard/tests/mail/bug80751.phpt

-imap
--CONFLICTS--
-imap
+curl


Technically, all of those tests also require JSON extension (which I know can't be disabled now; but who knows if this will always be the case)

Thanks, I overlooked it

we don't set json as it cannot be disabled (we actually removed it from EXTENSIONS and skip parts in all tests when to got always enabled).

Did we actually remove them? I don't really see any harm in having it in the EXTENSIONS section as that section is mainly used for Windows to load shared extensions if available and skip if not.

Yeah think it got removed from tests. It's not shared extension for Windows either so there is no point to specify it. It's like adding standard or spl there...

staabm · 2024-01-22T08:42:46Z

we are using mailhog for several years, so please take this as constructive feedback about problems mailhog has:

nearly not maintained
last time I looked at it, there was no support for issues in the tracker
when mails contain attachments you are running a use-cases which turn the webinterface to be super slaggy/slow
the chaos monkey feature can be enabled easily by any end-user by mistake, which makes testing super unreliable

we want to move to mailpit which is very similar, but seems more maintained and was created by its author mostly because of the above mentioned mailhog problems

SakiTakamachi · 2024-01-22T09:03:58Z

@staabm
Thanks for the feedback!

Yeah, mailhog hasn't been updated in a long time, so my friend recommended mailpit to me.

But since these tests are for Windows, I needed to include an executable in the SDK. For now, I am planning to include only the 64-bit version of the executable file in the SDK, but depending on the situation, we may need to add the 32-bit version.

I'm sorry if I overlooked it, but the mailpit Windows version executable should only support 64bit.

Personally, I am thinking of using mailhog for a while, and if there are no requests for the 32-bit version, I will decide that there is no problem with just the 64-bit version, and then suggest switching to mailpit.

iluuu1994 · 2024-01-22T09:29:57Z

@SakiTakamachi Just in case you're unaware, the Windows 32-bit build runs on 64-bit Windows. So it seems like there should be no issue using the 64-bit mailpit service in the 32-bit build. Maybe I'm missing some details, I did not follow the conversation.

SakiTakamachi · 2024-01-22T09:46:50Z

@iluuu1994
I see, in that case it might make sense to adopt mailpit. I'm going to replace it.

bukka

I added some suggestions for improvement but it looks good in general.

bukka · 2024-01-22T09:47:12Z

ext/standard/tests/mail/mailhog_utils.inc

+    $c = curl_init();
+    curl_setopt($c, CURLOPT_URL, 'http://localhost:8025/api/v2/search?kind=to&query='.$to);
+    curl_setopt($c, CURLOPT_RETURNTRANSFER, true);
+    $res = curl_exec($c);
+    curl_close($c);


why don't you just use following?

$res = file_get_contents("http://localhost:8025/api/v2/search?kind=to&query=$to");

that would remove unnecessary dependency on curl...

I completely forgot about it...

bukka · 2024-01-22T09:48:59Z

ext/standard/tests/mail/mailhog_utils.inc

+    return json_decode($res, true);
+}
+
+function getHeaders ($response)


we mostly try to follow PSR-12 in tests (not strictly but better not to do space before ( in this case)

bukka · 2024-01-22T09:50:16Z

ext/standard/tests/mail/mailhog_utils.inc

+        $c = curl_init();
+        curl_setopt($c, CURLOPT_URL, 'http://localhost:8025/api/v2/messages/'.$id);
+        curl_setopt($c, CURLOPT_CUSTOMREQUEST, 'DELETE');
+        curl_setopt($c, CURLOPT_RETURNTRANSFER, true);
+        curl_exec($c);
+        curl_close($c);


please use stream - in this case you will need to also pass stream context to set method

ext/standard/tests/mail/mail_variation_win.phpt

bukka · 2024-01-22T09:55:04Z

ext/standard/tests/mail/bug80751.phpt

-imap
--CONFLICTS--
-imap
+curl


we don't set json as it cannot be disabled (we actually removed it from EXTENSIONS and skip parts in all tests when to got always enabled).

bukka · 2024-01-22T09:58:25Z

ext/standard/tests/mail/mail_variation_win.phpt

+        $res &&
+        getFromAddress($res) === $from &&
+        getToAddresses($res)[0] === $to &&
+        getSubject($res) === $subject &&
+        getBody($res) === $message


as this is pretty much repeated everywhere, it might make sense to have a a function in utils for this. Maybe something like:

mailCheckResponse($res, $from, $to, $subject, $message)

Makes sense. I'll make it a function.

bukka · 2024-01-22T09:59:19Z

.github/scripts/windows/test_task.bat

@@ -113,6 +113,7 @@ curl -sLo hMailServer.exe https://www.hmailserver.com/download_file/?downloadid=
 hMailServer.exe /verysilent
 cd %APPVEYOR_BUILD_FOLDER%
 %PHP_BUILD_DIR%\php.exe -dextension_dir=%PHP_BUILD_DIR% -dextension=com_dotnet appveyor\setup_hmailserver.php


we can probably remove this like, right?

SakiTakamachi · 2024-01-22T17:07:24Z

The mailpit version has been completed, so it has been closed.

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

SakiTakamachi force-pushed the replace_imap_to_mailhog branch 18 times, most recently from 45b0a17 to a6c9cd4 Compare January 21, 2024 10:47

SakiTakamachi self-assigned this Jan 21, 2024

SakiTakamachi added the Category: CI label Jan 21, 2024

SakiTakamachi removed their assignment Jan 21, 2024

SakiTakamachi added 2 commits January 21, 2024 20:42

Replaced imap with mailhog and refreshed the test.

fc34aa3

wip

aad70ab

SakiTakamachi force-pushed the replace_imap_to_mailhog branch from a6c9cd4 to aad70ab Compare January 21, 2024 11:42

SakiTakamachi requested review from derickr, bukka, iluuu1994 and Girgias January 21, 2024 12:22

SakiTakamachi marked this pull request as ready for review January 21, 2024 12:25

SakiTakamachi requested a review from TimWolla as a code owner January 21, 2024 12:25

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

Girgias reviewed Jan 22, 2024

View reviewed changes

bukka reviewed Jan 22, 2024

View reviewed changes

SakiTakamachi mentioned this pull request Jan 22, 2024

Replaced imap with mailpit and refreshed the test. #13219

Closed

1 task

SakiTakamachi closed this Jan 22, 2024

SakiTakamachi mentioned this pull request Jan 26, 2024

Follow-up to remove IMAP ext #13248

Merged

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

Replaced imap with mailhog and refreshed the test. #13197

Replaced imap with mailhog and refreshed the test. #13197

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Girgias 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm 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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iluuu1994 commented Jan 22, 2024

Uh oh!

SakiTakamachi commented Jan 22, 2024

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!

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!

SakiTakamachi commented Jan 22, 2024

Uh oh!

Uh oh!

SakiTakamachi commented Jan 19, 2024 •

edited

Loading

SakiTakamachi commented Jan 21, 2024 •

edited

Loading

bukka Jan 22, 2024 •

edited

Loading

staabm commented Jan 22, 2024 •

edited

Loading

SakiTakamachi commented Jan 22, 2024 •

edited

Loading