Illuminate\Notifications\DatabaseNotification uses the $notifiable's database $connection

[emjayess]

Laravel Version

10.39

PHP Version

8.3

Database Driver & Version

MySQL 8.2.0

Description

The scenario is that the application leverages Laravel's and Eloquent's support for multiple databases, and by declaring the $connection for models which are in other than the primary laravel database (legacy databases, in this instance)..

The problem arises when the Notifiable model (Teacher) is stored in a secondary or legacy database, denoted by the Teacher model's $connection property...

Upon attempt at $teacher->notify(new ReminderClassCheckin());, I discovered that the database notification attempts to store itself using the notifiable Teacher model's connection, instead of in the 'default' laravel database connection.

Models in app/Models will use the primary database implicitly, without need of declaring a $connection property. Thus, that's the behavior I expected of this Framework model as well, but this turns out not to be the case.

---

As is, to make this work the way I want and expect, it's necessary to override \Illuminate\Notifications\{Notifiable, DatabaseNotifications, HasDatabaseNotifications}... overriding or replacing the traits, and extending the DatabaseNotification model class with the $connection property.

This all seems like it ought not be necessary. I'm a mere implementor, not a framework author, so it isn't immediately evident to me at which point in the lifecycle the database connection needs to be reset (or unset) to get the expected behavior; or, it's unclear to me why the behavior I'd expect is in fact, not supposed to be the expected behavior.

Steps To Reproduce

A laravel application with at least two database connections.

• notifications table in the first / primary database
• a notifiable model in secondary database
• a notification configured to save to database
• attempt to notify the notifiable

Behold:

[driesvints]

Thanks. I see you attempted a PR but sent it to the wrong branch. Laravel v10 still receives bug fixes. Can you attempt your PR at 10.x?

[emjayess]

Thanks. I see you attempted a PR but sent it to the wrong branch. Laravel v10 still receives bug fixes. Can you attempt your PR at 10.x?

That is not my PR. And at a glance, it does not address or resolve what I've reported. It's just a differently thought work-around.

Why close this issue without explanation or resolution?

[yourchocomate]

Thanks. I see you attempted a PR but sent it to the wrong branch. Laravel v10 still receives bug fixes. Can you attempt your PR at 10.x?

That is not my PR. And at a glance, it does not address or resolve what I've reported. It's just a differently thought work-around.

Why close this issue without explanation or resolution?

Isn't it the scenario that you're having notifications at primary connection and notifiable to another connection and want to have morph relation from different database?

[yourchocomate]

Thanks. I see you attempted a PR but sent it to the wrong branch. Laravel v10 still receives bug fixes. Can you attempt your PR at 10.x?

That is not my PR. And at a glance, it does not address or resolve what I've reported. It's just a differently thought work-around.

Why close this issue without explanation or resolution?

The fix is replacing the connection property for notifications

[emjayess]

Why does the framework's DatabaseNotification model leech the $connection from the related model? This is the bug.

[yourchocomate]

Author

That's not a bug I guess. Notifiable trait accessing the instance of notifiable model and creates a morphMany relation with DatabaseNotification Model which is expected as there is no definition of where you have created the notifications table

So, the PR offers an optional property to define which connection owns the notifications table. So, that you can work with several connections, even can create a separate connection only for the notifications table wihout having any problem

[emjayess]

That's not a bug I guess. Notifiable trait accessing the instance of notifiable model and creates a morphMany relation with DatabaseNotification Model which is expected as there is no definition of where you have created the notifications table

So, the PR offers an optional property to define which connection owns the notifications table. So, that you can work with several connections, even can create a separate connection only for the notifications table wihout having any problem

If this were true, it follows that no cross-database relationships would work, in absence of an explicitly declared $connection property on all of them.

Is this the assertion?

[emjayess]

Assert

I assert that framework models (e.g. \Illuminate\Notifications\DatabaseNotification) ought to be rightly expected to use the 'default' database connection specified in /config/database.php

And if there's to be a patch for overriding that – for example, for the inverse of the case I've described here – that it would best be designed as a configuration-based solution, such as publishing a /config/notifications.php in which the database connection for notifications table be declared.

[emjayess]

driesvints closed this as completed

irritating to have an issue summarily closed without being addressed or explained, @driesvints

[driesvints]

Calm down. Trying to read up 😅 I mistook you for being the same as @yourchocomate sorry.

[yourchocomate]

Assert

I assert that framework models (e.g. \Illuminate\Notifications\DatabaseNotification) ought to be rightly expected to use the 'default' database connection specified in /config/database.php

And if there's to be a patch for overriding that – for example, for the inverse of the case I've described here – that it would best be designed as a configuration-based solution, such as publishing a /config/notifications.php in which the database connection for notifications table be declared.

That's a good suggestion I guess. But I think notifications refers not only to the database channel.

[driesvints]

Left a comment on the PR. Will re-open this in the meantime.

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[driesvints]

Sorry, but right now it seems we're not undertaking any action here.

[emjayess]

The (faulty, I still assert) $connection inheritance takes place in HasRelationships @ newRelatedInstance($class), in the call stack.

[emjayess]

The (faulty, I still assert) $connection inheritance takes place in HasRelationships @ newRelatedInstance($class), in the call stack.

Works

as I'd expect, by blocking the implication that it should use the parent model's $connection:

• only for framework models, in this version

//  trait HasRelationships

    protected function newRelatedInstance($class)
    {
        return tap(new $class, function ($instance) {
            $instanceIsFramework = Str::contains(
                (new \ReflectionClass($instance))->getNamespaceName(),
                'Illuminate'
            );

            if (! $instance->getConnectionName() && ! $instanceIsFramework) {
                $instance->setConnection($this->connection);
            }
        });
    }

Also works

as I'd expect, by just altogether short-circuiting the entire problematic assumption, that the child class ought to leech the parent's $connection:

• would be for all models; app and framework

//  trait HasRelationships

    protected function newRelatedInstance($class)
    {
        return new $class;
    }

This seems unnecessary.

Notably, this DatabaseNotification does seem to be the only model class in the framework, at this time.