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

[9.x] SesTransport: use correct Tags argument #42390

Merged
merged 1 commit into from
May 16, 2022

Conversation

Tietew
Copy link
Contributor

@Tietew Tietew commented May 15, 2022

Illuminate\Mail\Transport\SesTransport uses SES v1's ses:SendRawEmail API but passes v2's EmailTags argument.
The call doesn't fail but the tags are not attached to the sent email.
(ConfigurationSet events don't include specified tags)

This PR fixes the parameter name to correct Tags.

Note: I didn't migrate to SesV2Client for backward compatibility.

Reference:
https://docs.aws.amazon.com/ja_jp/aws-sdk-php/v3/api/api-email-2010-12-01.html#sendrawemail
AWS SDK for PHP 3.x: SesClient->sendRawEmail()

I confirmed fired ConfigurationSet events include tags specified in Mailable's metadata() method.

class TestMail extends Mailable
{
    public function build()
    {
        return $this->metadata('env', config('app.env'))->text('emails.test');
    }
}

Mail::to(...)->send(new TestMail());

Before:

{
    "eventType": "Delivery",
    "mail": {
        ...
        "tags": {
            "ses:operation": ["SendRawEmail"],
            ...
        }
    },
    "delivery": ...
}

After:

{
    "eventType": "Delivery",
    "mail": {
        ...
        "tags": {
            "ses:operation": ["SendRawEmail"],
            ...
            "env": ["local"] // <- HERE
        }
    },
    "delivery": ...
}

@Tietew Tietew changed the title SesTransport: use correct Tags argument. [9.x] SesTransport: use correct Tags argument. May 15, 2022
@GrahamCampbell GrahamCampbell changed the title [9.x] SesTransport: use correct Tags argument. [9.x] SesTransport: use correct Tags argument May 15, 2022
@GrahamCampbell GrahamCampbell requested a review from driesvints May 15, 2022 20:20
@driesvints
Copy link
Member

Ping @kbond does this look good to you?

@driesvints driesvints marked this pull request as draft May 16, 2022 07:23
@kbond
Copy link
Contributor

kbond commented May 16, 2022

Good catch @Tietew!

does this look good to you?

I believe so, yes. I didn't realize there are multiple api versions. Symfony uses the AsyncAws client so it wasn't a straight copy in #41422.

@driesvints
Copy link
Member

@kbond Thanks!

@driesvints driesvints marked this pull request as ready for review May 16, 2022 15:00
@taylorotwell taylorotwell merged commit d2429ab into laravel:9.x May 16, 2022
@Tietew Tietew deleted the ses-transport-tags-argument branch May 16, 2022 16:04
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.

4 participants