Skip to content
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.4] Use parent connection if related model doesn't specify one #16103

Merged
merged 2 commits into from Nov 2, 2016
Merged

[5.4] Use parent connection if related model doesn't specify one #16103

merged 2 commits into from Nov 2, 2016

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Oct 25, 2016

In reference to this #9355 (comment)

Currently there's no way to query posts on the parent connection "non_default_connection":

User::on('non_default_connection')->first()->posts;

This PR will use the parent model connection if the related model doesn't have a connection specified.

@davisonja
Copy link

Is this going to be pushed back into 5.1 LTS as well?
If it resolves #9355 that would be great.

@themsaid
Copy link
Member Author

@davisonja not sure, 5.1 is technically closed for adding features, only bug fixes :)

@taylorotwell
Copy link
Member

Just for a reminder, what is the current behavior? It just uses the default connection null I'm guessing? Would there really be any way to fallback to this behavior if you wanted to in 5.4?

@themsaid
Copy link
Member Author

Yes it uses the default connection if not Model $connection was set.

With this PR there's no way to fallback to default no, unless you set the Model $connection to use the default connection. Not null, the default one.

@themsaid
Copy link
Member Author

themsaid commented Oct 26, 2016

The reason behind this change is multi-tenancy, to be able to do:

Song::on('band-123')->with('artist')->get();

This will bring the artists of tenant band-123 instead of the ones in the default connection.

@davisonja
Copy link

@themsaid True, I still feel it's a bug, though I concede that's primarily as I can't think of a use case for wanting

Song::on('band-123')->with('artist')->get();

to (always) return artists from the default connection; and I am currently using it in a situation where we need the artists to come from 'band-123'.

@BartHuis
Copy link

@reno1979

@reno1979
Copy link

reno1979 commented Oct 28, 2016

@themsaid @taylorotwell I would really appreciate this fix.

We 'clone' parts of our database models into multiple connections, and because they are the same we can use our Eloquent Models on each connection.

When working with an eloquent model instance on a specific connection, I did not expect that the related models of that instance would be using the applications default connection. Causing corrupted data in the database or errors.

We 'solved' it for now using a queue on a new thread, that changes the default database connection before the process starts. This is not a real solution and feels like a hack.

We would like to be able to retrieve :

$dataFromConnection1     = $DummyModel::on('connection1')->first();
$relatedDataConnection1  = $RelatedModel::on('connection1')->firstOrCreate(['description'=>'I exist in connection 1']);

$dataFromConnection2     = $DummyModel::on('connection2')->first();
$relatedDataConnection2  = $RelatedModel::on('connection2')->firstOrCreate(['description'=>'I exist in connection 2']);

$dataFromConnection3     = $DummyModel::on('connection3')->first();
$relatedDataConnection3  = $RelatedModel::on('connection3')->firstOrCreate(['description'=>'I exist in connection 3']);

// Attach related data
$dataFromConnection1->relatedData()->attach($relatedDataConnection1);
$dataFromConnection2->relatedData()->attach($relatedDataConnection2);
$dataFromConnection3->relatedData()->attach($relatedDataConnection3);

echo $dataFromConnection1->relatedData()->description;
/* I exist in connection 1 */
echo $dataFromConnection2->relatedData()->description;
/* I exist in connection 2 */
echo $dataFromConnection3->relatedData()->description;
/* I exist in connection 3 */

One note:
The on() method returns a Eloquent Builder instance, thus you should convert it back to the Model instance using getModel(). Is this correct?

It would also fix this issue on the translate extension:

dimsav/laravel-translatable#283

@BartHuis
Copy link

BartHuis commented Nov 1, 2016

@themsaid @taylorotwell what's the status on this one?

@taylorotwell taylorotwell merged commit bb15574 into laravel:master Nov 2, 2016
@BartHuis
Copy link

BartHuis commented Nov 3, 2016

why only 5.4 and not the current improvements on 5.3? when will 5.4 come out?

@BartHuis
Copy link

BartHuis commented Nov 3, 2016

here: https://laravel-news.com/2016/01/laravel-release-process/ i see it'll be december?

@GrahamCampbell
Copy link
Member

Might not be then. Probably more like January, though it's not been decided.

@BartHuis
Copy link

BartHuis commented Nov 3, 2016

@GrahamCampbell why not in the sub updates in 5.3?

@GrahamCampbell
Copy link
Member

This is a breaking change so can't go to 5.3.

@BartHuis
Copy link

BartHuis commented Nov 3, 2016

@GrahamCampbell aah, when changing composer to 5.4 we can allready use the development version of 5.4 right? because this is an important one to us...

@reno1979
Copy link

reno1979 commented Jan 13, 2017

We inserted the code from the patch (Eloquent/Model.php) , but can't seem to get it working.

$featureModel = new \App\FeatureGroup;

$featureModel->setConnection('otherConnection');

$featureModel->get();  // Ok we are receiving data from 'otherConnection'


$featureModel->create(['en'=>['name'=>"general"],'nl'=>['name'=>"algemeen"]]); 

// The created row is not in the 'otherConnection', but is placed in the default database.... 

@themsaid
Copy link
Member Author

@reno1979 but the example you shared doesn't involve related models, which is the purpose of this PR.

Can you please open a new issue with an example on how to generate the issue? But without using a package, just a fresh laravel app cause packages may have changed the default behaviour.

@reno1979
Copy link

@themsaid

Thank you for the quick reply.

We are not using any packages at the moment, but the create method keeps using the default db.
Our goal is to fill a related table (translations).

We are using artisan tinker, and tried the following:

\App\FeatureGroup::on('otherConnection')->getModel()->create(['name'=>'test']);
// a new row is inserted in the default db, but should be in the otherConnection db


/* OUR WORKAROUND */

$featureGroupModel = new \App\FeatureGroup;
$featureGroupModel->setConnection('otherConnection');
$featureGroupModel->save();

$featureGroupModel->id; // the given id is from the new table row in otherConnection db :)

@wells
Copy link
Contributor

wells commented Mar 24, 2017

Here's a helpful hint to setup a base model with this change now in place for 5.4.

If some of your models use an alternate connection, then you can override the default connection while still keeping it for all other models by having them all inherit from this class.

<?php

namespace App;

use Illuminate\Database\Eloquent\Model as BaseModel;

class Model extends BaseModel 
{
    /**
     * Get the current connection name for the model.
     *
     * @return string
     */
    public function getConnectionName()
    {
        return config('database.default');
    }
}

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.

None yet

7 participants