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

[8.x] Mail empty address handling #39035

Merged
merged 6 commits into from
Sep 29, 2021
Merged

Conversation

Synchro
Copy link
Contributor

@Synchro Synchro commented Sep 29, 2021

This PR is a follow-up to this discussion.

Short version: I have an optional bcc address set in config that all emailed messages are sent to. It works fine if that value is set, but if it's not, then Laravel just crashes. This PR makes all mail addressing functions (to, cc, bcc, replyTo, from) handle empty addresses cleanly. In the existing Laravel code, this code will fail if mail.bcc is not set in config:

Mail::to($user)
    ->bcc(config('mail.bcc'))
    ->send(new Message($user));

Error:

Attempt to read property "email" on null

The two patches (to setAddress() and hasRecipient()) use empty() to check the $address parameter, so that all empty/falsy values are handled, including false, [], null and ''.

Alternatives to this patch are not immediately obvious, for example, this doesn't work:

->bcc(config('mail.bcc', []))

because the config will only fall through to the default if the config value is null (missing from .env altogether), not just empty (present, but without a value, as is common in .env.example files). This works, but it's ugly and annoying to have to do every time, and you should not need to know the internals of the mail class in order to do this safely:

->bcc(config('mail.bcc') ?: []))

I note that there are no tests for bad inputs, only good ones, so I have added tests for these bad cases, and they fail without this patch.

I also noticed that there are no tests at all for the from() and hasFrom() methods, so I added a full set of those in the same style.

@GrahamCampbell GrahamCampbell changed the title Mail empty address handling [8.x] Mail empty address handling Sep 29, 2021
@taylorotwell taylorotwell merged commit 4de8ef4 into laravel:8.x Sep 29, 2021
@Synchro
Copy link
Contributor Author

Synchro commented Sep 29, 2021

Thank you!

victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Handle empty address inputs cleanly

* Tests for empty mail inputs

* Tests for from address

* Update Mailable.php

* Think this is simpler

* DRY, also test `false`

Co-authored-by: Taylor Otwell <taylor@laravel.com>
PHLAK pushed a commit to PHLAK/framework that referenced this pull request Oct 20, 2021
* Handle empty address inputs cleanly

* Tests for empty mail inputs

* Tests for from address

* Update Mailable.php

* Think this is simpler

* DRY, also test `false`

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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