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] Some refactoring on Eloquent builder #18775

Merged
merged 1 commit into from
Apr 11, 2017
Merged

[5.4] Some refactoring on Eloquent builder #18775

merged 1 commit into from
Apr 11, 2017

Conversation

themsaid
Copy link
Member

Extracted a new method that'll create a new model instance and set the connection to the same connection of the Builder instance:

public function newModelInstance($attributes = [])
{
    return $this->model->newInstance($attributes)->setConnection(
        $this->query->getConnection()->getName()
    );
}

Refactored the areas where this logic used to happen to call the method instead.

  • some minor docblock fixes.

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

Thanks

@gabrielkoerich
Copy link
Contributor

gabrielkoerich commented Jun 22, 2017

@themsaid This commit introduced a bug in my tests, if I create a model just using new Model() and save(), the connection is null, so all my comparisons using $model->is(...) doens't work anymore. Maybe we should use this new method in save() too? I don't know.

@themsaid
Copy link
Member Author

@gabrielkoerich I don't think this commit made a change to the code, we just extracted part of the code in a method.

@gabrielkoerich
Copy link
Contributor

gabrielkoerich commented Jun 22, 2017

@themsaid Yeap, that's weird. It was something between 5.4.18 and 5.4.19 (v5.4.18...v5.4.19), and the only commit that changed something with connections was this one... Do you know what else could it be? Thanks.

@gabrielkoerich
Copy link
Contributor

@themsaid I end up calling fresh() after creating the model and it's working.

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