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] Ensure subsiquent calls to Mailable->to() overwrite previous entries #45885

Merged

Conversation

AAllport
Copy link
Contributor

@AAllport AAllport commented Jan 31, 2023

Fixes a bug introduced by #45119 where subsequent calls to ->to() with a different name will not take effect.
We do this by reversing the collection before the call to unique and then reversing it back.

A test for this functionality has been added to MailMailableTest::testMailableSetsRecipientsCorrectly()

More context - #45119 (comment)

@taylorotwell
Copy link
Member

Can you explain how the reversing fixes the issue?

@AAllport
Copy link
Contributor Author

AAllport commented Jan 31, 2023

So, when adding additional addresses via ->to() or any of the other methods, the entire array is passed through ->unique()
Collection::unique() (when you provide a key) will iterate and keep a record of the keys it has seen. (Src)
Each iteration will check the key against the keys it's already seen and reject if it's already there.
As such, unique will keep the first instance of the array containing the key.

Before this change, calling the following:

$mailable->to("taylor@laravel.com")->to("taylor@laravel.com", "Taylor Otwell")

Will result in the following to array:

[
    ["email"=>"taylor@laravel.com", "name"=>null]
]

Whereas intuitively, the name should be overridden by the last provided instance. Eg:

[
    ["email"=>"taylor@laravel.com", "name"=>"Taylor Otwell"]
]

In place of a uniqueLast() function, we have to reverse the array to keep the last.
This broke the tests that rely on the order of the to/cc/bcc array, so we flip it back afterwards.

Frustratingly, this caught us out when updating from an older version as ->assertMailTo($email,$name) was throwing as the name was null, despite calling ->to($email,$name) in our build() function.

@taylorotwell taylorotwell merged commit 846744c into laravel:9.x Jan 31, 2023
@AAllport
Copy link
Contributor Author

Awesome! Thanks for merging 🙌

@AAllport AAllport deleted the prefer-latter-addtion-to-recipients branch January 31, 2023 14:53
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.

2 participants