[11.x] Added notifiable connection property to specify database connection for notifications

[yourchocomate]

Description:

This pull request addresses the issue where Laravel's notification system does not behave as expected when dealing with models stored in secondary or legacy databases (introduced at issue #50564). Currently, notifications attempt to store themselves using the database connection of the notifiable model, even if it's not the primary Laravel connection.

To solve this problem, this pull request introduces a new $notifiableConnection property to the Illuminate\Notifications\Notifiable trait. This property allows developers to specify the database connection to be used for notifications, overriding the connection specified in the notifiable model as new feature for 11.x

Usage:

class Teacher extends Model
{
    use Notifiable;

    protected $connection = 'secondary-connection';

    protected $notifiableConnection = 'primary-connection';
}

Testing:

• Integration tests have been conducted to verify the behavior of notifications across different database connections.

Any feedback or suggestions for improvement are welcome.

[driesvints]

Please send this to 10.x

[yourchocomate]

Please send this to 10.x

Thought, it could be a feature for 11.x

Am, sending this to 10.x

[yourchocomate]

Please send this to 10.x

Should I make any modifications or it's okay to be opened in 10.x

Suggestions are appreciated!

[driesvints]

Sorry I thought this was a bug fix. Happy to re-open this for 11.x

[yourchocomate]

Sorry I thought this was a bug fix. Happy to re-open this for 11.x

Thank you, am not making PR for 10.x then.

Or, Should it be available for both 10.x and 11.x?

[me-shaon]

@yourchocomate You should fix the 'StyleCI' issue in your code: https://github.styleci.io/analyses/MPRQ7v

[yourchocomate]

@yourchocomate You should fix the 'StyleCI' issue in your code: https://github.styleci.io/analyses/MPRQ7v

Thanks, I'm updating it.

[driesvints]

After reviewing this, I'm not sure this is the right approach as this will set the connection on the model class that has HasDatabaseNotifications applied. What you want to do instead is set the connection on the retrieved DatabaseNotification instances. But that can't be done here.

[driesvints]

This is a tough nut to crack. I think the only way to do this is to introduce a new config setting somewhere where the default database connection of the DatabaseNotification can be defined.

[yourchocomate]

Should I work on it?

I will convert this PR to draft then.

[driesvints]

@yourchocomate want to see what @taylorotwell has to say first.

[emjayess]

This is a tough nut to crack. I think the only way to do this is to introduce a new config setting somewhere where the default database connection of the DatabaseNotification can be defined.

Is it understood why it doesn't default to default (which could be changed by introducing a publishable config for it)? It's not clear to me the behavior of the connection resolver... leeching the notifiable model's $connection, I might presume, has to do with the trait setup?

---

EDIT

if I just add this to Illuminate\Notifications\DatabaseNotification, I'm able to both generate notifications on the notifiable, and retrieve them as expected:

public function __construct()
{
    $this->connection = config('database.notifications') ?: config('database.default');
}

[yourchocomate]

This is a tough nut to crack. I think the only way to do this is to introduce a new config setting somewhere where the default database connection of the DatabaseNotification can be defined.

Is it understood why it doesn't default to default (which could be changed by introducing a publishable config for it)? It's not clear to me the behavior of the connection resolver... leeching the notifiable model's $connection, I might presume, has to do with the trait setup?

---

EDIT

if I just add this to Illuminate\Notifications\DatabaseNotification, I'm able to both generate notifications on the notifiable, and retrieve them as expected:

public function __construct()
{
    $this->connection = config('database.notifications') ?: config('database.default');
}

Thought of that fix but made my mind think if this approach is not standard approach of explicitly accessing config like that.

Waiting for the suggestion for right approach from @taylorotwell

[driesvints]

Atm I do not immediately see how DatabaseNotification takes over the connection of its parent model.

if I just add this to Illuminate\Notifications\DatabaseNotification

You shouldn't overwrite a model's constructor as that's not supported and can lead to a variety of bugs.

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.