[5.4] Accessing model through relationship defaults to current connection

[Xethron]

• Laravel Version: 5.4.16
• PHP Version: 5.6.30
• Database Driver & Version:

Description:

If you have a model using a non-default connection, and has a relationship to a model with using the default connection, it will continue to use the first model's connection unless another connection was specified.

Steps To Reproduce:

Create two models, and specify a non-default connection in the first:

<?php

class Images extends \Illuminate\Database\Eloquent\Model
{
    protected $connection = 'imagedb';

    public function user()
    {
        return $this->belongsTo(User::class);
    }
}

<?php

class User extends \Illuminate\Database\Eloquent\Model
{
    public function images()
    {
        return $this->hasMany(Images::class);
    }
}

Then try to fetch the user through the relationship: $image->user->name and you will get some error like:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'imagedb.users' doesn't exist (SQL: select * from `users` where `users`.`id` = 1 and `users`.`deleted_at` is null limit 1)

To fix this, you have to specify the connection in the User model as well: protected $connection = 'default';

This used to work on all versions from 4.0 up until 5.3.

[Xethron]

This seems to have been implemented as a "feature" of sorts... https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Concerns/HasRelationships.php#L489

Changing

protected function newRelatedInstance($class)
{
    return tap(new $class, function ($instance) {
        if (! $instance->getConnectionName()) {
            $instance->setConnection($this->connection);
        }
    });
}

to

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

fixes the issue, but it does break testRelationsWithVariedConnections:
https://github.com/laravel/framework/blob/5.4/tests/Database/DatabaseEloquentModelTest.php#L1033

So I'm not sure why this was implemented, as the model using the default connection should always use the default connection regardless of where its accessed from?

[Xethron]

There is a bigger chance that if a model without a connection is accessed via a relationship, that the model is using the default connection, and not the connection of the model its accessed through. Otherwise this model won't be usable if not accessed via the relationship...

Also, I see this was already reported as #17624 (Edit: Actually, no, that one relates to a subquery, which is actually how Laravel has been working to date...)

[Xethron]

So after looking around a bit, this seems to have been implemented here: #16103 to allow you to change the connection for a model and its relationships like so User::on('non_default_connection')->first()->posts;.

Problem is, we have a few big tables in different databases, and reading from db's off of other systems. So we actually do connect to about 5 different DB's (seen as outside of the system) and one DB that we maintain.

Now even though this feature allows you to change the connection for all related models, we can't change the default connection in our tests anymore as we physically need to set the default connection on every single model now in order for cross database relations to still work. And because we did this, we can't use this feature because there is no way of "activating or deactivating" it once the default connection is set to a value.

How do we fix this?

I think a better way would be to save the "default" connection of the model when it gets changed. That way, when you access a relation of that model, you can check if its "default connection" was changed to something else. If it was changed, check if the relating model had the same connection, and only if they where the same, update the connection for the relating model.

I can try play with this to see if I can come up with something, but would like to know what you think first.

[themsaid]

If you want to always load a related model on a specific connection you can just define it in the model class, otherwise the parent model connection will be used.

So in your case all you need is just define the default connection explicitly in your models.

[deleugpn]

I had a similar problem when dealing with dynamic table. I ended up overriding the newRelatedInstance method.

    /**
     * By overriding this method, we allow the relationship to accept a built (and modified) object. This way we can modify the table's name.
     * @param $class
     * @return mixed
     */
    protected function newRelatedInstance($class) {
        if (is_string($class))
            $class = new $class;
        
        return tap($class, function ($instance) {
            if (!$instance->getConnectionName()) {
                $instance->setConnection($this->connection);
            }
        });
    }

and then

    /**
     * Offer Segment relationship for any model that has a segment_{letter} model.
     * @param $segmentName
     * @return mixed
     */
    public function segment($segmentName) {
        $instance = new Segment();
        $instance->setTable($segmentName);
        return $this->belongsTo($instance, $segmentName);
    }

[Xethron]

So, this new feature allows you to change the connection on the fly, maybe for a staging DB or something. So by the looks of it, if you have two models on the default db, you can quickly switch to another connection by simply changing the connection of the first model. This requires both models to use the default connection.

This however is not possible for any models who have their connections defined. However, looking at the above scenario, it seems that we actually want the ability to switch out the connection on the fly for all models that are using the same connection.

So if we have two production databases, and a set of models on the default connection, and another set on db2, if I change the connection of a model using db2, all its relationships should also change. If however I fetch a relationship on the default connection from a db2 model, it should continue using the default connection.

The way this feature has been implemented makes it impossible to use the feature if a project makes use of cross database relationships, as this will require them to specify the connection on EVERY model (which isn't elegant at all), and because of that, connections can't be changed.

It also means that updating the default connection in your config/env will have no effect, making it more difficult for switching it out for testing.

It feels like this feature hides itself, and think the use case for cross database relationships are more popular than someone who wishes to switch out an entire connection to another db with the same schema.

Proposed fix

Store the default connection of a model. If its changed, check if related models had the same default connection, and only change if true.
Most production systems won't need this feature however, and would probably cause more problems in the long run. Perhaps adding a flag to the models to say "use parent connection info", or maybe an array of parent classes might be a better idea...

[Xethron]

@deleugpn Took me a few seconds to realize what you actually did, and I think thats actually quite neat. Allows the developer to add the modifications they want (like setting the connection/table) instead of the framework deciding that for you.

[themsaid]

That change was introduced in 5.4 because it was reported as a bug in previous versions, previously there was no way to instruct laravel to load the related model from the parent connection, that's why we made it this way, by default it loads from the parent model connection unless you explicitly state otherwise.

All you have to do is explicitly set the default connection on your Model instance and it'll always load the model from this model.

[shadoWalker89]

@themsaid

previously there was no way to instruct laravel to load the related model from the parent connection

How about defining the custom connection on the related model instead of setting the $connection to 'default' on all the models using the default connection.

IMHO, i think that most of the models in an application will be using the default connection.

In my case, i'm upgrading an app from 5.3 where i have

32 models using the default connection

4 models using a custom connection

I think it's a lot easier to set the custom connection on 4 models instead of setting the connection to default on 32 models which is the obvious behavior

---

Edit:

At least in my mind the old behavior make more sense than the new one in 5.4.

Let's assume an application with 20 models
15 of them are on the default connection
5 of them are on the custom connection

4 out of the 5 models using custom connection are not related to any other model in the app.
The fifth model using a custom connection (called SuperModel) is related to all of the 15 models using the default connection.

So in order for the following code to work

SuperModel::with(['model1OnDefaultConnection', 'model2OnDefaultConnection', ...]);

I have to set the connection to default on all 15 models.

This seems a bit weird to me.

In fact, the example that i made isn't just an example, it's from my real life, in production application that i'm upgrading from 5.3 (32 models on default 4 on custom)

I'm running a multi tenancy application, where i have one shared database between all tenants. (Default connection)

Each tenant has a custom connection that will hold settings, meta data ... models, that are not really related to any other model in the application. (Custom connection)

The only model from the custom connection that is related to models on the default connection, is an Audit model that is used to track changes.

So in order for me to use 5.4 i need to set the connection to default on each model that i have, which is by the way the obvious behavior and shouldn't be done.

[themsaid]

SuperModel::with(['model1OnDefaultConnection', 'model2OnDefaultConnection', ...]);

This will work just fine, but SuperModel::on('anotherConnection')->with() will load the related models on anotherConnection unless you explicitly say they should be run on the default one.

This makes more sense to me since it gives you full control over how you load the models.

[shadoWalker89]

The fact that i have to go define

protected $connection = 'default'; On models using the default connection doesn't make any sense.

---

Edit :

We laravel users talk all the time how easy it is to deal with databases by just extending a Class.
And now, i have to define that the connection is default on models using the default connection ? Which by the way, the majority of models on an application will be on default connection.

[shadoWalker89]

Another problem is see is when using packages.
A model from a package (on default) is used in relation with a model in the application (on a connection other then default)

[Xethron]

@themsaid

That change was introduced in 5.4 because it was reported as a bug in previous versions
This makes more sense to me since it gives you full control over how you load the models.

I'd like to present the scenario where this is still a bug:

When using a model from a different connection, it incorrectly updates the connection of a related model on the default connection.

To fix this, I have to set the default connection, which breaks both changing the default connection in my .env file, and Model::on('anotherConnection') no longer works as expected when swapping connections out on the fly.

This change makes Eloquent less elegant, breaks the ability to change default connection info in .env, and the previous bug report is still open because "fixing" the new bugs brings back the old bug and more...

This change doesn't feel like it gives you full control, but rather as if it takes control away.

How can this be done differently?

1. Specify if a relationship should use the parent connection

Implement a way for developers to activate/deactivate this on a per relationship basis. Maybe something like: return $this->belongsTo($instance)->useParentConnection();

2. Only update connection of relationships if by default, they had the same connections

This feature also only makes sense if:

1. The models default connection was changed (aka, doing Model::on())
2. The related model had the same default connection (aka, the default set connection on the model was changed)

class User extends Model
{}

class Images extends Model
{
    protected $connection = 'images';

    public function user()
    {
        return $this->belongsTo(User::class);
    }
}

In the above example, the default connection for User is the default connection, where for Images, its default connection is 'images'. So when you do Images::on('old_images')->with('user'), then user should still use the default connection, but any relationship using the 'images' connection should be updated to 'old_images'.

So perhaps doing something like:

return tap($class, function ($instance) {
    if ($this->defaultConnection != $this->connection && $this->defaultConnection === $instance->getConnectionName()) {
        $instance->setConnection($this->connection);
    }
});

3. Leave it all in the hands of the developer

I also really like @deleugpn's approach that allows you to construct the model beforehand, changing any parameters you need, and then passing them in. This means on any model you wish to activate this, you can simply do:

public function user() {
    $instance = new User();
    if (!$instance->getConnectionName()) {
        $instance->setConnection($this->connection);
    }
    return $this->belongsTo($instance);
}

This approach leaves you fully in control, but probably removes some of the "elegant" nature. None the less, I think having the ability to pass in constructed models is a great idea and does open a lot of doors.

---

I agree that in some ways this new feature makes more sense, but I also think the way it was implemented could have been done better. The above examples could solve some of those problems, while still allowing the developer to use the feature on both default AND non-default connections, without forcing them to chose between one or the other.

If you can give me your thoughts on this, I can try come up with a solution and send a PR for further discussion. An actual code example might help drive the conversation a little better.

[themsaid]

So to summarise this in order to start taking actions, our issues here can be solved if reading from the parent connection could be optional?

For example

User::on('non-default-connection', true)->with(['posts']);
// This loads the posts on the `non-default-connection` unless the 
// POST model has a connection set explicitly.

User::on('non-default-connection', false)->with(['posts']);
// This loads the posts on the `default` connection unless the 
// POST model has a connection set explicitly.

This is not a suggested syntax or anything, I'm just demonstrating how this can be solved, can you please let me know if that's what you're asking for. A way to switch between 5.4 behaviour and 5.3 behaviour?

@Xethron @shadoWalker89

[deleugpn]

@themsaid will you also consider to allow the developer to have power over the object with the decision structure on newRelatedInstance?

[themsaid]

@deleugpn I'm not quite sure I understand what you mean in your comment.

[deleugpn]

@themsaid Proposed Solution number 3 on @Xethron's last comment.

Basically boils down to this:

    /**
     * By overriding this method, we allow the relationship to accept a built (and modified) object. This way we can modify the table's name.
     * @param $class
     * @return mixed
     */
    protected function newRelatedInstance($class) {
        if (is_string($class))
            $class = new $class;
        
        return tap($class, function ($instance) {
            if (!$instance->getConnectionName()) {
                $instance->setConnection($this->connection);
            }
        });
    }

[Xethron]

@themsaid Exactly. Also, I wouldn't exclude models that have been set explicitly, as you might want to use this feature on those models. There should be a more verbose way of turning it off/on.

I think also, limiting it only change when "::on()" was used, and when the two models where on the same connection previously, would make the feature work as most would expect. (instead of the current implementation of including models that have the $connection property hard-coded)

@deleugpn is referring to the ability to pass in an already constructed object into a relationship like so:

public function user() {
    $instance = new User();
    return $this->belongsTo($instance);
}

[shadoWalker89]

@themsaid Thanks for the answer mohamed.
I fully understand how to migrate from 5.3 to 5.4 and that is not the problem.

The problem is that, in 5.3 made more sense then now. I'm not saying that the 5.3 is perfect, maybe it could be improved with the way @Xethron said.

Upgrading a large app from 5.3 (like mine) will be a living hell unless i go and set the connection to default on each model using the default connection, which is a weird thing to do.

Please take into consideration that i'm not participating in this issue because of the upgrade of my app (i already upgraded it to 5.4) But i'm simply sharing my point of view, and try to demonstrate how this feature is a weird.

[themsaid]

Hey everyone, we're getting closer to 5.5 so it's the last chance to introduce a breaking change, to me it really isn't an issue to explicitly define the default connection name on the models that will always work on the default connection, I personally don't feel we need to do changes just to eliminate this step. So here after months of the 5.4 behaviour I suppose you already upgraded your apps, do you still feel there's a need for such change?

[jrseliga]

@themsaid It does feel anti-Laravel to require a developer to explicitly set

protected $connection = 'default'

Since default is the assumed connection if none is provided on the Model.

I think it's counter-intuitive that retrieving a Model instance operates in one manner when using that class directly, but in another manner when querying relationships.

Wouldn't modifying Illuminate\Database\Eloquent\Model@getConnectionName() to return the default connection if the current Instance of the Model doesn't have the property connection set be a way to solve this as a none breaking change?

    public function getConnectionName()
    {
        return $this->connection ?? config('database.default');
    }

[fernandobandeira]

@jrseliga Nope, it would break the current behavior:

protected function newRelatedInstance($class)
{
    return tap(new $class, function ($instance) {
        if (! $instance->getConnectionName()) {
            $instance->setConnection($this->connection);
        }
    });
}

Since getConnectionName would always return something it would never use the connection of the parent model as it currently is.

Personally I think the current behavior is acceptable.

[Xethron]

My biggest issue with it is not that it requires you to specify the default model, but by specifying the default model, you opt out of this behaviour completely...

I have a few ideas that I'll put to a test this weekend, and hopefully have a PR ready by Sunday evening.

[deleugpn]

I am now facing this problem with a pretty annoying complication:

• During tests, my default connection is called sqlite;
• During application runtime, my default connection is called master (for master/tenant app);

If I set the parent connection to master, all my tests breaks.
If I omit the connection name, the related object one is assumed, leading to wrong database connection.
if I change my tests to use main as connection name, my Laravel Dusk tests fail because the SQLite connection is based on project path.
I can't just override the newRelatedInstance because my models are outside of Laravel and don't have helper functions to get the environment.

Quite annoying this problem.

[themsaid]

For suggestions and feature requests let's use the https://github.com/laravel/internals repo.

[juancarlosestradanieto]

Hi everyone, I use postgres with many schemas, I put in my model in the table attribute the schema, so for my schema "personas" and my table "contratos" has the attribute protected $table = "personas.contratos"; and I always get the right connection. I'm not sure if it helps, it may help if the tables are in the same database, but I hope so.

[projct1]

Its still not workig =(

class User extends Eloquent
{
    protected $connection = 'mysql';
}

class Asterisk extends Eloquent
{
    protected $connection = 'asterisk';

    public function admin()
    {
        return $this->setConnection('mysql')->belongsTo('App\User', 'dst', 'phone_internal');
    }
}

Asterisk::whereHas('admin')->first(); //not working, getting error `Base table or view not found: 1146 Table 'asterisk.users' doesn't exist`

Asterisk::first()->admin; //works

[MuriloChianfa]

@themsaid It does feel anti-Laravel to require a developer to explicitly set

protected $connection = 'default'

Since default is the assumed connection if none is provided on the Model.

I think it's counter-intuitive that retrieving a Model instance operates in one manner when using that class directly, but in another manner when querying relationships.

Wouldn't modifying Illuminate\Database\Eloquent\Model@getConnectionName() to return the default connection if the current Instance of the Model doesn't have the property connection set be a way to solve this as a none breaking change?

    public function getConnectionName()
    {
        return $this->connection ?? config('database.default');
    }

help me so much, thx