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

[5.6] Only set ID on NotificationFake if there is no ID set #23470

Merged
merged 3 commits into from Mar 9, 2018

Conversation

Projects
None yet
4 participants
@hotmeteor
Contributor

hotmeteor commented Mar 9, 2018

This PR makes the NotificationFake@sendNow method behave the same way as the sending functionality in the NotificationSender, in which the sender checks if the notification ID has already been set before setting it.

hotmeteor added some commits Mar 9, 2018

@hotmeteor hotmeteor changed the title from Only set ID on NotificationFake if there is no ID set to [5.6] Only set ID on NotificationFake if there is no ID set Mar 9, 2018

@ninjaparade

This comment has been minimized.

Contributor

ninjaparade commented Mar 9, 2018

This might be the greatest idea since your ->doesntExist() on the Query builder that the frontend guy claimed as his own. 💥 💣

@taylorotwell taylorotwell merged commit 8a06746 into laravel:5.6 Mar 9, 2018

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -174,7 +174,9 @@ public function sendNow($notifiables, $notification)
}
foreach ($notifiables as $notifiable) {
$notification->id = Uuid::uuid4()->toString();
if (! $notification->id) {

This comment has been minimized.

@36864

36864 Mar 9, 2018

Contributor

This might sound a bit nitpicky, but wouldn't checking for null be preferable here to allow '0' to be used as an ID?

This comment has been minimized.

@hotmeteor

hotmeteor Mar 9, 2018

Contributor

Well a UUID would never be 0, so not sure that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment