active-record icon indicating copy to clipboard operation
active-record copied to clipboard

Proposal: Work with the related models in AR like one-entity.

Open asamats opened this issue 5 years ago • 16 comments

I don't know how to name it, I just try to explain by example. We have models like this:

class User extends ActiveRecord
{
    // ...
    public function getFiles(): ActiveQuery
    {
        return $this->hasMany(UserFile::class, ['id' => 'file_id']);
    }
    // ...
}

class UserFile extends ActiveRecord
{
    // ...
}

And we need to load/save/update/etc User and UserFiles in one transaction because we wanna has integrity DB.

What we do: https://www.yiiframework.com/doc/guide/2.0/en/input-multiple-models But it is not a right way to save/update integrity models.

Yes, we can use AR/DB transaction methods from Yii but I think must work by default when we work with related models/data.

And also we type same code for save related models.

My proposal:

When we wanna create/update/save/delete related models we must use eager-loading for get models:

User::find()-> with(['files'])->one();

... forms and etc.

And when we get data by request from browser we just use load function:

$user->load(Yii::$app->request->post())

And this function load all related models if it was loaded. And after that - validate & save all models like one Entity.

BUT: For security reason we must allow to work like this only for one "Entity". What I mean - UserFiles without User doesn't have any value, User and UserFiles - it is one Entity. And for this we must have agreement that - If a model name has parent name (UserFiles has User) it must interpreted like one-single entity.

Thanks. I will be grateful for the reasoned criticism and additions. And yes, I'm ready to implement this if it was be accepted.

UPDATE (2019-05-22):

  1. dependent models by model name (User -> [User]File) - reject My proposal - use static method that return array where key is relation name and value is FQCN for dependent model Example ['files' => 'app/models/UserFile']
  2. eager-loading isn't necessary

asamats avatar May 16 '19 09:05 asamats

We should add too much hidden work behind the scene mainly to set the foreign keys, and it is not a good thing to have not full control of what happens during save.

I usually adopt the next example, simple, clear and linear.

$user = ... fill user model ...
$userFile = ... fill userFile model ... (without setting foreign key because it is not known at this time)

$transaction = \Yii::$app->db->beginTransaction();

try
{
	// save the parent model (user). If it fails, an exception is thrown
	if($user->save())
	{
		// set the foreign key
		$userFile->user_id = $user->id;

		// save the child model (userFile). If it fails, an exception is thrown
		if($userFile->save())
		{
			$transaction->commit();
		}
		else
		{
			throw new \Exception('error on UserFile Model ' . print_r($userFile->firstErrors));
		}
	}
	else
	{
		throw new \Exception('error on User Model ' . print_r($user->firstErrors));
	}
}
catch(\Exception $excp)
{
	$transaction->rollback();
}

fcaldarelli avatar May 16 '19 23:05 fcaldarelli

Please, look at https://github.com/yiisoft/active-record/pull/70 and make comments. Thanks.

asamats avatar May 22 '19 10:05 asamats

@FabrizioCaldarelli I think you agree with me that it doesn't need for app, it is just some code for AR (or DB layer) and it is repeatable and same for all dependent models.

asamats avatar May 22 '19 10:05 asamats

@asamats Sure, but I'd not include automatically this feature inside AR or DB layer.

fcaldarelli avatar May 22 '19 10:05 fcaldarelli

@FabrizioCaldarelli Yes, I agree about automatically feature that is way I change my proposal. Now it is customizable.

asamats avatar May 22 '19 10:05 asamats

@asamats I could have dependent models, but I could want to not update them when I save the parent model.

fcaldarelli avatar May 22 '19 10:05 fcaldarelli

@FabrizioCaldarelli What if we add a flag or new method for save parent and all depend models (if them added and updated)?

asamats avatar May 22 '19 11:05 asamats

@asamats It is not a AR responsability to do this work. Again, think at more complex cases, where you need to "sync" models, and not only save. For example, you start with 3 UserFile model related to 1 UserFile. Then, I want to save only 2 UserFile (so the other UserFile should be deleted). Your code in this case not work, because it will only save (without changes) the two models, without delete the other.

fcaldarelli avatar May 22 '19 12:05 fcaldarelli

@asamats In this case it do. But anyway, I see that core yii developer doesn't approve.

Thanks for your comments, them was helpful.

asamats avatar May 22 '19 12:05 asamats

@asamats I'm not sure why you're closing this issue. It's labeled under discussion and other developers/community member may still not had the chance to review & think about your proposal..

machour avatar May 22 '19 13:05 machour

I think it's not a good idea to try to add this functionality by default to all the ARs. Just think about this cases:

  • How to process the situation when we have an array of related models? When should we add some new UserFiles or find existent in DB or delete them?
  • If we must eager load all the related models there is a huge problem when we has thousands of them.

IMHO this logic must be implemented situationally as it is project-specific.

viktorprogger avatar May 27 '19 14:05 viktorprogger

@viktorprogger

  1. It's solvable. I see two way:
  • by find PK. Usually we have 1-100 related models, and it's fast to compare in the loop.
  • using model's PK for key in the related's array. And tagged all depend models - deletable(checked) and when updated models by incoming data - deletable(unchecked). After that it's simple to delete/update/add dependent models.
  1. Like I have updated my proposal - it was mistake.

I don't see any problem with implementation. Question is - want we have in AR this is functionality?

asamats avatar May 27 '19 15:05 asamats

I still think

  • this logic mustn't be default (if it will be realized in any variant)
  • it is not AR responsibility at all. AR is already overloaded (I mean yii2 version). If it will be implemented it should be a separate service or at least a decorator. Maybe it is reasonable to make another composer library for this.
  • it could be a good kick-up for startups, but a very big headache for big old projects.

viktorprogger avatar May 29 '19 07:05 viktorprogger

@asamats why don't you create an extension to achieve it?

fcaldarelli avatar May 29 '19 08:05 fcaldarelli

@FabrizioCaldarelli because we have similar exts. I don't see any reason to create one other.

asamats avatar May 29 '19 08:05 asamats

@viktorprogger "it could be a good kick-up for startups, but a very big headache for big old projects." Did you research my draft-pull-request? It doesn't break anything and by default don't change AR behavior. You can use this functionality when you want.

What about AR overloaded - I can't say nothing, but when you proposal to use decorator pattern - I'm confused. Do you realize how to do this? I will be happy if you push draft-pull-request.

"this logic mustn't be default" - it doesn't.

P.S. Yii never will be like Symfony. It's his best side.

asamats avatar May 31 '19 12:05 asamats