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] Set connection while retrieving models #18769

Merged
merged 1 commit into from
Apr 11, 2017
Merged

[5.4] Set connection while retrieving models #18769

merged 1 commit into from
Apr 11, 2017

Conversation

themsaid
Copy link
Member

In #17395 we moved the model's create() and forceCreate() methods to the Builder to allow something like User::on('myCustomConnection')->create();, before that creating a model on a different collection was a bit hacky.

After that it was pretty obvious that hydrate() and fromQuery() can be moved to the builder instance as well, since we won't have to pass that $connection argument and we just get it from the Builder instance, that's when 1ae29d9 was committed.

However in that commit we removed the argument but forgot to pass the connection name from the Builder instance, leaving the connection as null when we retrieve a model.

In this PR we simply set the connection using $this->query->getConnection()->getName(), just like we do in create() and forceCreate().

This will solve the failing test in #18760 without having to re-introduce the $connection argument to the hydrate() method.

@adamwathan
Copy link
Contributor

Perfect, thanks @themsaid!

@adamwathan
Copy link
Contributor

Looks like there's a good opportunity to extract this to a method:

$instance = $this->model->newInstance($attributes)->setConnection(
    $this->query->getConnection()->getName()
);

Appears 7 times! 😄

@taylorotwell taylorotwell merged commit 7392014 into laravel:5.4 Apr 11, 2017
@taylorotwell
Copy link
Member

@themsaid if you see a good way to extract that method go for it 😄

@themsaid
Copy link
Member Author

@adamwathan yeah looks like a great candidate for having an own method, will PR that in a while :)

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.

3 participants