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.7] Added chunkById support in relations. #26919

Merged
merged 4 commits into from Dec 20, 2018

Conversation

Projects
None yet
3 participants
@dkulyk
Copy link
Contributor

dkulyk commented Dec 19, 2018

Pivot data doesn't attached to model If you use Builder::chunkById.

This PR is fixing this and using actual of relation keys for chunking.

Closes #26816.
Related to laravel/nova#152

For 5.6 need separate PR?

dkulyk added some commits Dec 19, 2018

@staudenmeir

This comment has been minimized.

Copy link
Contributor

staudenmeir commented Dec 20, 2018

I'm wondering about the BelongsToMany implementation:
Should we also use $this->getRelated()->[...] for $column and $alias (as HasManyThrough does)?

In a UserRole relationship:

  • Your implemenation: role_user.role_id
  • My suggestion: roles.id

It would be more consistent and in basically every case both columns contain the same value. The only exception would be a relationship that doesn't join the related model's primary key (e.g. roles.slug). But I'm not even sure what the expected/correct behavior would be in a case like this.

@taylorotwell taylorotwell merged commit 6e71b4e into laravel:5.7 Dec 20, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dkulyk dkulyk deleted the dkulyk:57_chunkbyid_in_relations branch Dec 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment