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

Decode mime headers #45

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Decode mime headers #45

merged 1 commit into from
Sep 9, 2020

Conversation

xico42
Copy link
Contributor

@xico42 xico42 commented Sep 4, 2020

The email headers in the response body from mailhog are encoded, which makes working with utf-8 characters non-intuitive.

A new abstraction for the headers was created in order to encapsulate the parsing and decoding of mime headers.

As an example, the following test snippet will fail as the current SubjectSpecification does not take into account the decoding of the subject.

<?php
use GuzzleHttp\Client as GuzzleClient;
use Http\Adapter\Guzzle6\Client as GuzzleAdapter;
use Http\Message\MessageFactory\GuzzleMessageFactory;
use Illuminate\Support\Facades\Mail;
use rpkamp\Mailhog\MailhogClient;
use rpkamp\Mailhog\Specification\SubjectSpecification;

public function testUf8()
{
    $adapter = new GuzzleAdapter(new GuzzleClient());
    $requestFactory = new GuzzleMessageFactory();
    $client = new MailhogClient($adapter, $requestFactory, env('MAILHOG_API_BASEURL'));
    $client->purgeMessages();


    $subject = 'Mailhog é muito bom mesmo: açãoéôÍ';
    // This is encoded as: Mailhog =?utf-8?Q?=C3=A9?= muito bom mesmo: =?utf-8?Q?a=C3=A7=C3=A3o=C3=A9=C3=B4=C3=8D?=

    Mail::to(['foo@bar.com'])->send(new HtmlMailable($subject, 'foobar'));

    $messages = $client->findMessagesSatisfying(new SubjectSpecification($subject));

    // This assertion fails
    $this->assertCount(1, $messages);
}   

The web UI of mailhog implements some logic to decode mime headers and display the correct text to the user as can be seen here in the unescapeFromMime function.

In PHP this implementation is way easier because we have a built-in function for this.

@xico42
Copy link
Contributor Author

xico42 commented Sep 4, 2020

I've also taken the freedon to fix a broken test: it_should_release_a_message

@rpkamp
Copy link
Owner

rpkamp commented Sep 5, 2020

Thanks! I'm wondering if we should apply this function inside the method that parses the subject though, so it's always decoded, not just in the specifiction. What do you think?

@xico42
Copy link
Contributor Author

xico42 commented Sep 5, 2020

Well thought, idd it makes more sense.

For the user of this client it is not very intuitive to have the subject encoded like this. It will prevent a lot of unexpected behaviours in the future.

I will update the PR to do this decoding when parsing the subject.

@xico42 xico42 changed the title Improves the SubjectSpecification Decode mime headers Sep 7, 2020
The email headers in the response body from mailhog are encoded, which
makes working with utf-8 characters non-intuitive.

A new abstraction for the headers was created in order to encapsulate
the parsing and decoding of mime headers.
@xico42
Copy link
Contributor Author

xico42 commented Sep 9, 2020

I've made some changes, and now the PR should be ready:

  • The decoding is now applied to every header, all the headers are encoded
  • I created a Headers class to encapsulate the parsing and access to headers
  • The test I thought it was broken (it_should_release_a_message), it was broken only in my environment, so i reverted the changes to it
  • The phpmd 2.9.0 version contains a bug: Fatal error in AbstractLocalVariable phpmd/phpmd#816, so I've constrained the version in composer.json to a previous one
  • I fixed all the issues related to code style and static-analysis

@rpkamp
Copy link
Owner

rpkamp commented Sep 9, 2020

Wow, you really went above and beyond there! Awesome, thanks!

@rpkamp rpkamp merged commit 46a4b37 into rpkamp:master Sep 9, 2020
@rpkamp
Copy link
Owner

rpkamp commented Sep 9, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants