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

Check hasMany guess early #830

Merged
merged 2 commits into from
Jan 20, 2021
Merged

Check hasMany guess early #830

merged 2 commits into from
Jan 20, 2021

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jan 19, 2021

Join has simillar guessing, but as it operates on foreign_table instead of Model, we can not check early there

@mvorisek mvorisek force-pushed the no_implicit_their_field_in_ref branch from a8f56de to da006b0 Compare January 20, 2021 09:50
@mvorisek mvorisek changed the title No guess for "their_field" in Has{One,Many}::ref Check hasMany guess early Jan 20, 2021
@mvorisek mvorisek marked this pull request as ready for review January 20, 2021 09:52
Copy link
Collaborator

@georgehristov georgehristov left a comment

Choose a reason for hiding this comment

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

This version I like

->addMoreInfo('their_fallback_field', $theirField);
}

return $theirField;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe assign it to $this->their_field?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And rename $theirField to $theirFieldName for consistency...

Copy link
Member Author

@mvorisek mvorisek Jan 20, 2021

Choose a reason for hiding this comment

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

no - we should not materialize guesses if not done on construction time

@mvorisek mvorisek added the RTM label Jan 20, 2021
@mvorisek mvorisek merged commit c72260c into develop Jan 20, 2021
@mvorisek mvorisek deleted the no_implicit_their_field_in_ref branch January 20, 2021 10:08
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.

2 participants