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.2] Fix morphTo without SoftDeletes #13806

Merged
merged 2 commits into from
Jun 1, 2016
Merged

[5.2] Fix morphTo without SoftDeletes #13806

merged 2 commits into from
Jun 1, 2016

Conversation

acasar
Copy link
Contributor

@acasar acasar commented Jun 1, 2016

The whole approach with the cloned query sadly doesn't work in some edge cases. Instead we need to take a similar approach approach as we do with "has" queries (merging where clauses manually).

I added one more test to expose the issue (#13762) and refactored the implementation.

@taylorotwell @phroggyy

Anže Časar added 2 commits June 1, 2016 13:01
* @param \Illuminate\Database\Query\Processors\Processor $processor
* @return void
*/
public function useConnection(ConnectionInterface $connection,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this helper method since it's not needed any more. It was added here #13784 and hasn't been tagged yet.

@phroggyy
Copy link
Contributor

phroggyy commented Jun 1, 2016

Shouldn't we merge more than just wheres? There are lots of types here, such as joins

@acasar
Copy link
Contributor Author

acasar commented Jun 1, 2016

We currently only merge wheres in has queries too, so it should be enough for now.

@tiran133
Copy link

tiran133 commented Jun 1, 2016

Thank you this fixes the problem for me.

@phroggyy
Copy link
Contributor

phroggyy commented Jun 1, 2016

@acasar: why not just go through the keys of the bindings array? Basically

foreach($this->query->getBindings() as $type => $binding) {
    $query->{$type} = $this->query->{$type}
}

Or similar

@acasar
Copy link
Contributor Author

acasar commented Jun 1, 2016

@phroggyy Can't do it like you're suggesting since binding keys are not named the same as query parameters. As I said, for now I took this function (for has queries) and adjusted it accordingly: https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Builder.php#L989

If we decide to merge other clauses, we should do it in both places with another PR, since it's actually unrelated issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants