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.5] Update HasManyThrough Documentation #3306

Merged
merged 1 commit into from
May 9, 2017

Conversation

phroggyy
Copy link
Contributor

@phroggyy phroggyy commented May 9, 2017

This updates the documentation for HasManyThrough relations to clarify how the different keys are used, and to document the addition of laravel/framework#19114.

I'm not quite happy with the (in my opinion) too technical wording of the explaining paragraph, so I will gladly accept proposals for change.

This updates the documentation for `HasManyThrough` relations to clarify how the different keys are used, and to document the addition of laravel/framework#19114.

I'm not quite happy with the (in my opinion) too technical wording of the explaining paragraph, so I will gladly accept proposals for change.
@taylorotwell
Copy link
Member

Wait, so the last argument is for the intermediate table? That's kind of annoying because now the argument order goes... (model, model, intermediate, final, final, intermediate)... it should be model, model, intermediate, final, intermediate, final.

@phroggyy
Copy link
Contributor Author

phroggyy commented May 9, 2017

Yes. We can revert the previous one, and I'll re-PR, or I can submit a patch if you want that changed. The reason for it was to minimise breaking changes. Right now, the only BC break is in the HasManyThrough class. Also, if I'm not mistaken, the way it currently goes is Model, Model, intermediate, final, local, intermediate. localKey does not refer to the final if memory serves me right

@taylorotwell
Copy link
Member

I think it's OK.

@taylorotwell taylorotwell merged commit 445f2a0 into laravel:master May 9, 2017
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.

2 participants