New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). #19893

PowerGamer1 · 2023-07-13T12:52:54Z

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Fixed issues	#11000, #12743

Equivalent of https://laravel.com/docs/10.x/eloquent-relationships#lazy-eager-loading in Eloquent ORM.

Should be mentioned in Yii2 guide.

what-the-diff · 2023-07-13T12:53:08Z

PR Summary

Introduction of Eager Loading Methods
These changes introduce two new methods BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). These methods provide the functionality to pre-load related data for existing primary model instances. This enhancement can significantly improve application performance as it reduces the number of required queries to the database when retrieving associated data. Therefore, it's a beneficial update, particularly for applications with complex relational data structures.

codecov · 2023-07-13T12:55:48Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (505fd5a) 48.94% compared to head (02ed808) 48.95%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #19893   +/-   ##
=======================================
  Coverage   48.94%   48.95%           
=======================================
  Files         445      445           
  Lines       42817    42826    +9     
=======================================
+ Hits        20956    20964    +8     
- Misses      21861    21862    +1

Files	Coverage Δ
framework/db/BaseActiveRecord.php	`77.77% <88.88%> (+0.18%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

framework/db/BaseActiveRecord.php

samdark · 2023-07-14T14:48:45Z

Would you please add tests for the method?

PowerGamer1 · 2023-07-14T16:57:07Z

Would you please add tests for the method?

Done.

mtangoo · 2023-07-14T17:25:49Z

framework/db/BaseActiveRecord.php

+    public static function loadRelationsFor(&$models, $relationNames, $asArray = false)
+    {
+        // ActiveQueryTrait::findWith() called below assumes $models array is non-empty.
+        if (empty($models)) {


Why not if (!empty($models)) and avoid empty return statement which raises Codecov warning?

It will make the code less readable IMO:

your variant uses extra logical operator (which makes expression more complex for human understanding);

the ! operator is easily missable at a glance;

my variant has a clearly visible useful work of a function (last line) and a clearly separated condition when it can be skipped; your variant would have these tangled up making it harder to understand what is the normal function flow and what are the exceptions in it.

Making the code less readable just to shut up a warning in some imperfect external tool is a bad idea.

But shouldn't it cover the test, for when the model is empty?

But shouldn't it cover the test, for when the model is empty?

You mean if there should be a test for a loadRelationsFor([], /*...*/) call? I don't think this test will be very useful.

But it is testing the code, in the case that the model is empty.

But it is testing the code, in the case that the model is empty.

So you just want to do the testing for the sake of doing testing even if it doesn't test anything useful at all? If you disagree, please show us how such test could possibly look and explain its usefulness.

@PowerGamer1 when things breaks for empty models with some bug in the code (let say someone else modified your now empty return to something else, how will we detect it? That's where this test comes to rescue. Currently sound like useless, but it will make code "future proof" sort of!

@PowerGamer1 ... let say someone else modified your now empty return to something else, how will we detect it?

At present, testing a single return statement is useless. If, in the future, as you say someone replaces/modifies this return statement with a MORE COMPLEX piece of code - then at that point in time testing such MORE COMPLEX piece of code might become warranted. Obviously, testing of such (currently non-existent) more complex code path should be done when it is introduced by the person who introduced it.

Also, if I were you (i.e. engine maintainer), I would be much more concerned with testing infinitely more complex and currently completely untested pieces of code (such as ActiveQueryTrait::findWith() and myriad others). In fact, the only reason I am currently writing any tests at all is the fact that new shortcut methods rely on such untested functionality - i.e. I don't really test NEW code, I have to test already EXISTING code.

P.S. That test you want me to write - have you even imagined how exactly it will look? What exactly are you going to assert (like assertTrue() etc.) in it?

PowerGamer1 · 2023-07-15T11:44:36Z

It looks to me that the code relies on the "inverseOf" definition of the related ActiveRecord hasOne/hasMany relation. Is that correct? And if so, what happens when there is no "inverseOf" relation defined?

No such reliance on the inverseOf at all. The new methods just call (in a convenient way) the same existing functionality that is executed when you perform the eager loading using with() in a call like:

Customer::find()->with('orders.items')->all();

That example above works without defining inverse relation and so do new methods which you can see in the tests (neither Customer::getOrders() nor Order::getItems() relation in the tests uses inverseOf).

rhertogh · 2023-07-15T13:09:27Z

@PowerGamer1 Yes, when I dived deeper into it I came to the same conclusion and removed my comment. But your response beat me to it 😄.

samdark · 2023-07-27T09:00:40Z

@PowerGamer1 All good. Would you please resolve conflicts so I can merge it?

PowerGamer1 · 2023-07-27T10:26:39Z

@PowerGamer1 All good. Would you please resolve conflicts so I can merge it?

Done.

samdark · 2023-08-18T11:23:28Z

Please resolve the conflicts so I can hit the merge button. Thanks.

samdark · 2023-08-18T11:23:39Z

Please resolve the conflicts so I can hit the merge button. Thanks.

PowerGamer1 · 2023-08-18T13:23:14Z

@samdark Conflicts resolved.

samdark · 2023-08-26T04:16:54Z

Damn :( Conflicts again. @PowerGamer1 need your help.

PowerGamer1 · 2023-10-09T17:08:06Z

You might want to merge this soon as I will not be able to rebase this commit to latest master for much longer (as I will be leaving github due to forced 2FA policy).

bizley · 2023-10-13T09:52:39Z

@PowerGamer1 could you resolve conflicts one more time? I'll merge this immediately after. Sorry for the long time waiting.

…loadRelationsFor().

PowerGamer1 · 2023-10-13T10:58:46Z

@PowerGamer1 could you resolve conflicts one more time? I'll merge this immediately after. Sorry for the long time waiting.

@bizley No worries, conflicts resolved.

bizley · 2023-10-13T11:04:07Z

Thank you. Sorry to hear you are leaving GH.

xicond · 2023-10-14T03:48:09Z

I'm wandering, is this support nested relationship like dot notation

samdark reviewed Jul 14, 2023

View reviewed changes

framework/db/BaseActiveRecord.php Outdated Show resolved Hide resolved

framework/db/BaseActiveRecord.php Outdated Show resolved Hide resolved

samdark requested review from a team July 14, 2023 14:46

samdark added the pr:request for unit tests Unit tests are needed. label Jul 14, 2023

PowerGamer1 force-pushed the fix12743 branch from f36bd46 to b387652 Compare July 14, 2023 16:51

mtangoo reviewed Jul 14, 2023

View reviewed changes

PowerGamer1 force-pushed the fix12743 branch from b387652 to 7980720 Compare July 14, 2023 19:57

PowerGamer1 force-pushed the fix12743 branch from 7980720 to 7f8997f Compare July 17, 2023 20:09

samdark removed the pr:request for unit tests Unit tests are needed. label Jul 27, 2023

samdark approved these changes Jul 27, 2023

View reviewed changes

samdark added this to the 2.0.49 milestone Jul 27, 2023

PowerGamer1 force-pushed the fix12743 branch from 7f8997f to 7ceb320 Compare July 27, 2023 10:22

PowerGamer1 force-pushed the fix12743 branch from 7ceb320 to eafb70c Compare August 2, 2023 17:40

PowerGamer1 force-pushed the fix12743 branch from eafb70c to 13ebdb1 Compare August 10, 2023 11:09

PowerGamer1 force-pushed the fix12743 branch from 13ebdb1 to 34c8609 Compare August 18, 2023 11:54

PowerGamer1 force-pushed the fix12743 branch from 34c8609 to debc4a7 Compare August 26, 2023 07:58

bizley modified the milestones: 2.0.49, 2.0.50 Aug 29, 2023

PowerGamer1 force-pushed the fix12743 branch 2 times, most recently from ba168ec to 9e3595f Compare September 7, 2023 09:12

bizley modified the milestones: 2.0.49.1, 2.0.50 Oct 5, 2023

PowerGamer1 force-pushed the fix12743 branch from 9e3595f to 7fdf23c Compare October 9, 2023 16:59

New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::…

02ed808

…loadRelationsFor().

PowerGamer1 force-pushed the fix12743 branch from 7fdf23c to 02ed808 Compare October 13, 2023 10:24

bizley merged commit 4fe40f9 into yiisoft:master Oct 13, 2023
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). #19893

New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). #19893

PowerGamer1 commented Jul 13, 2023

what-the-diff bot commented Jul 13, 2023

codecov bot commented Jul 13, 2023 •

edited

Loading

samdark commented Jul 14, 2023

PowerGamer1 commented Jul 14, 2023

mtangoo Jul 14, 2023

PowerGamer1 Jul 14, 2023

terabytesoftw Jul 14, 2023

PowerGamer1 Jul 14, 2023

terabytesoftw Jul 14, 2023

PowerGamer1 Jul 15, 2023

mtangoo Jul 15, 2023 •

edited

Loading

PowerGamer1 Jul 15, 2023

PowerGamer1 commented Jul 15, 2023

rhertogh commented Jul 15, 2023

samdark commented Jul 27, 2023

PowerGamer1 commented Jul 27, 2023

samdark commented Aug 18, 2023

samdark commented Aug 18, 2023

PowerGamer1 commented Aug 18, 2023

samdark commented Aug 26, 2023

PowerGamer1 commented Oct 9, 2023

bizley commented Oct 13, 2023

PowerGamer1 commented Oct 13, 2023

bizley commented Oct 13, 2023

xicond commented Oct 14, 2023

New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). #19893

New methods: BaseActiveRecord::loadRelations() and BaseActiveRecord::loadRelationsFor(). #19893

Conversation

PowerGamer1 commented Jul 13, 2023

what-the-diff bot commented Jul 13, 2023

PR Summary

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

samdark commented Jul 14, 2023

PowerGamer1 commented Jul 14, 2023

mtangoo Jul 14, 2023

Choose a reason for hiding this comment

PowerGamer1 Jul 14, 2023

Choose a reason for hiding this comment

terabytesoftw Jul 14, 2023

Choose a reason for hiding this comment

PowerGamer1 Jul 14, 2023

Choose a reason for hiding this comment

terabytesoftw Jul 14, 2023

Choose a reason for hiding this comment

PowerGamer1 Jul 15, 2023

Choose a reason for hiding this comment

mtangoo Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

PowerGamer1 Jul 15, 2023

Choose a reason for hiding this comment

PowerGamer1 commented Jul 15, 2023

rhertogh commented Jul 15, 2023

samdark commented Jul 27, 2023

PowerGamer1 commented Jul 27, 2023

samdark commented Aug 18, 2023

samdark commented Aug 18, 2023

PowerGamer1 commented Aug 18, 2023

samdark commented Aug 26, 2023

PowerGamer1 commented Oct 9, 2023

bizley commented Oct 13, 2023

PowerGamer1 commented Oct 13, 2023

bizley commented Oct 13, 2023

xicond commented Oct 14, 2023

codecov bot commented Jul 13, 2023 •

edited

Loading

mtangoo Jul 15, 2023 •

edited

Loading