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

Add failing test for is() method #18760

Closed
wants to merge 3 commits into from
Closed

Add failing test for is() method #18760

wants to merge 3 commits into from

Conversation

adamwathan
Copy link
Contributor

For some reason, retrieving a model from the database isn't setting it's connection name, so if you create a model, retrieve it, and use is() to find out if it's the same model, it fails.

@adamwathan
Copy link
Contributor Author

So it looks like the primary break happened here, where the connection parameter was removed. It was still being provided in getModels, but not actually getting applied:

1ae29d9

Oddly, I also had to switch $this->model->getConnectionName() to $this->query->getConnection()->getName() to get the right connection name, but that doesn't appear to be reverting any other change; it's always used $this->model->getConnectionName() in the past 🤔

Some other unit tests are broken now due to missing mocked methods, but will look at those next.

@themsaid
Copy link
Member

I've opened a PR to fix this issue and included more details about the changes that lead to it: #18769

Here's what I found:

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.

@pschmidt88
Copy link

I'm having the same issue in 5.3

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.

4 participants