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

Deduplicate load methods and assert only one row can be loaded if expected #846

Merged
merged 10 commits into from
Apr 7, 2021

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Apr 6, 2021

no bc break

also add Model::loadOne() and Model::tryLoadOne() methods

@mvorisek mvorisek changed the title Deduplicate load methods and verify only one row if expected Deduplicate load methods and assert only one row can be loaded if expected Apr 6, 2021
@mvorisek mvorisek marked this pull request as ready for review April 6, 2021 13:29
@mvorisek mvorisek force-pushed the load_one branch 3 times, most recently from 4b2c891 to 03a2a83 Compare April 6, 2021 14:09
@mvorisek mvorisek removed the BC-break label Apr 6, 2021
@mvorisek mvorisek force-pushed the load_one branch 5 times, most recently from d9a17bd to 09cf4fd Compare April 6, 2021 14:49
@mvorisek mvorisek force-pushed the load_one branch 3 times, most recently from 7d2d413 to 00551ee Compare April 6, 2021 16:05
@mvorisek mvorisek force-pushed the load_one branch 3 times, most recently from 1379862 to bd7c6b3 Compare April 6, 2021 17:10
Copy link
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.

Quite a lot of changes, but LGTM

{
$load = $model->action('select');
$load->limit(1);
$query->limit($id === self::ID_LOAD_ANY ? 1 : 2);
Copy link
Member

@DarkSide666 DarkSide666 Apr 7, 2021

Choose a reason for hiding this comment

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

smart move with limit(2) :)
I think that will not be very bad for performance if it tries to select 2 records instead of one.
Only question is - what if table will actually have only one record? Then it will not be able to detect issue here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, performance should be fine

Only question is - what if table will actually have only one record? Then it will not be able to detect issue here.

that is expected and not only if table have not one record, but also if conditions match only one record :)

@DarkSide666 DarkSide666 merged commit 5c75154 into develop Apr 7, 2021
@DarkSide666 DarkSide666 deleted the load_one branch April 7, 2021 13:36
@mvorisek mvorisek added the MAJOR label Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants