yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.9k forks source link

Feature: Eager loading in ActiveRecord::refresh() #20277

Open chriscpty opened 1 day ago

chriscpty commented 1 day ago

As of current, ActiveRecord::refresh() and BaseActiveRecord::refresh() only refresh the record itself and lose all relations. This is the expected behaviour, but if you then need to iterate over nested relations afterwards, lazy loading them leads to an excessive amount of DB queries.

My proposal would thus be to add an optional parameter with to refresh:

public function refresh ($with = [])

The implementation in ActiveRecord uses the corresponding model's ActiveQuery anyway, so it might as well call ->with(...$with) on it in the process.

Q A
Yii version dev-master
PHP version 8.1
Operating system Debian 12

I'd be happy to implement this, but want to be sure this makes sense and I haven't missed anything relevant before :)

samdark commented 1 day ago

I find it interesting. @yiisoft/yii2, @yiisoft/reviewers thoughts?

bizley commented 1 day ago

This would be great, but if we want to add this in Yii 2 we need to do this with method arguments discovery, like Symfony does sometimes (for BC).

samdark commented 1 day ago

What's "method arguments discovery"?

bizley commented 1 day ago

Sorry, my term :P I mean this func_get_args() inside to allow flexibility.

chriscpty commented 1 day ago

I don't think I see why that'd be necessary instead of just having the $with argument have a default value (which as far as I can tell is supported for all PHP versions yii supports)? Existing implementations that don't use this argument still work then, and while overrides may show a warning, they won't break either.

The only case of breaking I could see (that would happen afterwards though) is if a user overrides refresh and adds differently named arguments, then tries to pass with as a named argument - but I don't think that's a BC break.

bizley commented 1 day ago

Adding argument to refresh(), even optional one, will break user's apps that extend this method.

chriscpty commented 1 day ago

Just tested it and yeah, you are right - I misremembered whether PHP can deal with arguments being missing in overrides :D

func_get_args() it is then

schmunk42 commented 1 day ago

Should go into 2.2, if implemented.

samdark commented 22 hours ago

2.2 may break compatibility.