Skip to content

Conversation

@DarkGhostHunter
Copy link
Contributor

@DarkGhostHunter DarkGhostHunter commented Sep 1, 2021

What?

Allows using dynamic properties like Eloquent Models to the AnonymousNotifiable (on demand notifications).

use Illuminate\Support\Facades\Notification;
use App\Notifications\GuitarShipped;

Notification::route('mail', '[email protected]')
    ->with(['name' => 'John', 'instrument' => 'guitar'])
    ->notify(new GuitarShipped('JP45'));

This solves the problem when a notification expects data from the notifiable, as the AnonymousNotifiable doesn't support gracefully chaining properties, as it difficult to manually set each of them. Worse case scenario: making another notification.

This way is totally safe to get injected data from the notifiable.

public function toMail($notifiable)
{
    return (new MailMessage)->subject("Hey $notifiable->name, your guitar is on route!");
}

The with() method supports Arrayable objects, so you can use a whole Model if you feel lazy.

use Illuminate\Support\Facades\Notification;
use App\Notifications\GuitarShipped;
use App\Models\Client;

$client = Client::find(477);

Notification::route('mail', '[email protected]')
    ->with($client) // I would used `$client->only('name', 'instrument'), but you do you.
    ->notify(new GuitarShipped('JP45'));

How?

I just slapped the Fluent helper into the AnonymousNotifiable and added the with() method to merge multiple attributes in one go.

BC?

Please...

Notes

  1. The Notifications package requires the Support package, so there is no sin to use duck-tape Fluent in my opinion.
  2. I didn't set with() into the Facade, so it still forces the developer to route first and merge properties later.
  3. The other alternative I do is to clone the Model into the Notification, but feels very hacky to me.

@BrandonSurowiec
Copy link
Contributor

One consideration. Fluent allows non-existent methods to add an attribute. Previously, non-existent methods would throw an error.

Notification::route('mail', '[email protected]')
    ->name('Ludwig van Beethoven')
    ->instrument('Piano')
    ->notify(new GuitarShipped('JP45'));

This behavior may or may not be desired. If it isn't then the __call method should be overridden in the AnonymousNotifiable class.

@DarkGhostHunter
Copy link
Contributor Author

I consider that an improvement.

@derekmd
Copy link
Contributor

derekmd commented Sep 1, 2021

  1. With Fluent added, this typo expression would become a silent no-op that may only be detected by an integration test:
    Notification::route('mail', '[email protected]')
        ->rout('nexmo', '5555555555')
        ->local('not_locale')
        ->notif(new NotDeliveredNotification);
  2. If $name, $instrument, or $client are required to build the notification, they can be passed into the GuitarShipped constructor with less message-passing magic.

@DarkGhostHunter
Copy link
Contributor Author

  1. With Fluent added, this typo expression would become a silent no-op that may only be detected by an integration test:

Well, that's a valid point. I'll disable the __call() by overriding it.

  1. If $name, $instrument, or $client are required to build the notification, they can be passed into the GuitarShipped constructor with less message-passing magic.

When you use a notifiable Model you are still end up using message-passing magic. Using with() allows to rewrite the Notification to keep using the Notifiable data.

@taylorotwell taylorotwell merged commit 9f9b737 into laravel:8.x Sep 2, 2021
@driesvints driesvints mentioned this pull request Sep 2, 2021
driesvints added a commit that referenced this pull request Sep 2, 2021
@driesvints
Copy link
Member

We're reverting this: #38651. I agree with @derekmd that a better way would be to pass the arguments to the notification constructor.

taylorotwell pushed a commit that referenced this pull request Sep 2, 2021
* Revert "Apply fixes from StyleCI"

This reverts commit cdb14c7.

* Revert "[8.x] Adds dynamic properties to `AnonymousNotifiable` (#38628)"

This reverts commit 9f9b737.
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Adds Fluent and `with()` to the AnonymousNotifiable.

* Style changes

* Disables dynamic calls to AnonymousNotifiable.

* Update AnonymousNotifiable.php

Co-authored-by: Taylor Otwell <[email protected]>
victorvilella pushed a commit to cdsistemas/framework that referenced this pull request Oct 12, 2021
* Revert "Apply fixes from StyleCI"

This reverts commit cdb14c7.

* Revert "[8.x] Adds dynamic properties to `AnonymousNotifiable` (laravel#38628)"

This reverts commit 9f9b737.
@DarkGhostHunter DarkGhostHunter deleted the feat/fluent-notification branch November 25, 2021 17:46
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.

5 participants