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

[6.x] Fixes #31124 #31125

Merged
merged 3 commits into from Jan 14, 2020
Merged

[6.x] Fixes #31124 #31125

merged 3 commits into from Jan 14, 2020

Conversation

@shehi
Copy link
Contributor

shehi commented Jan 14, 2020

This fixes the bug where pivot attribute name is customized. When customized, that attribute isn't "pivot" anymore (and doesn't get filtered out), and as a result of it remaining in the list of relationships to be loaded during refresh(), model attempts to load these pivots as actual relationships, and this naturally fails.

Fixes #31124

@driesvints driesvints changed the title Fixes #31124 [6.x] Fixes #31124 Jan 14, 2020
@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Jan 14, 2020

Heya, can you please provide an explanation what this fixes instead of linking to an issue? Also please add a test that proves any bug gets fixed.

shehi added 2 commits Jan 14, 2020
@shehi

This comment has been minimized.

Copy link
Contributor Author

shehi commented Jan 14, 2020

@GrahamCampbell , @taylorotwell please kindly review this, and merge as soon as possible. This is a blocker for my project, so a fast release would be great.

@shehi

This comment has been minimized.

Copy link
Contributor Author

shehi commented Jan 14, 2020

@driesvints , done with description. Good enough? :)

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Jan 14, 2020

@shehi we'll need a test as well.

@shehi

This comment has been minimized.

Copy link
Contributor Author

shehi commented Jan 14, 2020

@driesvints , I think you already have a test, it just ran with "pivot" attribute name, instead of some customized one. If this isn't the right test, please point me in right direction - tests there are too "messy" for my small brain.

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Jan 14, 2020

@shehi the fact that nothing breaks with these changes is an indication that this isn't tested. We'll need a test close to that one I suspect.

@shehi

This comment has been minimized.

Copy link
Contributor Author

shehi commented Jan 14, 2020

I won't be able to create that test. Don't really have time at this point in time. Please close this one if it can't continue, I will just rename my stuff to "pivot".

@taylorotwell taylorotwell merged commit 942e2c5 into laravel:6.x Jan 14, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shehi

This comment has been minimized.

Copy link
Contributor Author

shehi commented Jan 14, 2020

@taylorotwell , @driesvints , I know it wasn't quite "ethical" to force you guys into merging it like this, I really don't have time right now for test writing. Please accept my apologies.

@shehi shehi deleted the shehi:patch-1 branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.