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

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

Open
asamats opened this issue May 16, 2019 · 16 comments
Open

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

asamats opened this issue May 16, 2019 · 16 comments

Comments

@asamats
Copy link

asamats commented May 16, 2019

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
@fcaldarelli
Copy link
Member

fcaldarelli commented May 16, 2019

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();
}

@samdark samdark closed this as completed May 20, 2019
@samdark samdark reopened this May 20, 2019
@yiisoft yiisoft deleted a comment from yii-bot May 20, 2019
@samdark samdark transferred this issue from yiisoft/yii-core May 20, 2019
@asamats
Copy link
Author

asamats commented May 22, 2019

Please, look at #70 and make comments. Thanks.

@asamats
Copy link
Author

asamats commented May 22, 2019

@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.

@fcaldarelli
Copy link
Member

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

@asamats
Copy link
Author

asamats commented May 22, 2019

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

@fcaldarelli
Copy link
Member

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

@asamats
Copy link
Author

asamats commented May 22, 2019

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

@fcaldarelli
Copy link
Member

fcaldarelli commented May 22, 2019

@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.

@asamats
Copy link
Author

asamats commented May 22, 2019

@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 asamats closed this as completed May 22, 2019
@machour
Copy link
Member

machour commented May 22, 2019

@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..

@asamats asamats reopened this May 23, 2019
@viktorprogger
Copy link
Contributor

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.

@asamats
Copy link
Author

asamats commented May 27, 2019

@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?

@viktorprogger
Copy link
Contributor

viktorprogger commented May 29, 2019

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.

@fcaldarelli
Copy link
Member

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

@asamats
Copy link
Author

asamats commented May 29, 2019

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

@asamats
Copy link
Author

asamats commented May 31, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants