Skip to content

Require array in reference/join defaults#820

Merged
mvorisek merged 3 commits intodevelopfrom
no_str_defaults
Dec 30, 2020
Merged

Require array in reference/join defaults#820
mvorisek merged 3 commits intodevelopfrom
no_str_defaults

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Dec 30, 2020 •
edited
Loading

hasOne/hasMany was nice with string/object, but allowing string violates seed definition

if we however allow a seed for Model directly (without ['model' => ...]) then it can not be distinguished from type only (array vs. array)

thus this impl. is strict, but the only one simple and reliable

@mvorisek mvorisek changed the title Table must be specified with table key in defaults Require array in reference/join defaults Dec 30, 2020
@mvorisek mvorisek force-pushed the no_str_defaults branch 2 times, most recently from 9e122c4 to c328144 Compare December 30, 2020 00:56
@mvorisek mvorisek mentioned this pull request Dec 30, 2020
Merged
DarkSide666
DarkSide666 approved these changes Dec 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

This is quite big BC break, but for consistency and good code sake it should be like that.

One thing to also think about is maybe rename join master_field -> our_field and foreign_field -> their_field? Like we have in references? That would be more consistent then too. Of course that would also be BC break, but ...

@georgehristov
Copy link
Copy Markdown
Collaborator

georgehristov commented Dec 30, 2020 •
edited
Loading

One thing to also think about is maybe rename join master_field -> our_field and foreign_field -> their_field? Like we have in references? That would be more consistent then too. Of course that would also be BC break, but ...

This idea I would support. For consistency we should also consider ourField and theirField names in both references and joins

@mvorisek
Copy link
Copy Markdown
Member Author

mvorisek commented Dec 30, 2020

I have no resources to pursue another renames, but I like that idea to.

@mvorisek mvorisek merged commit 48ca2aa into develop Dec 30, 2020
@mvorisek mvorisek deleted the no_str_defaults branch December 30, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@georgehristov georgehristov Awaiting requested review from georgehristov

Assignees

No one assigned

Labels

BC-break

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mvorisek @georgehristov @DarkSide666