-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Comments
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();
} |
Please, look at #70 and make comments. Thanks. |
@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 Sure, but I'd not include automatically this feature inside AR or DB layer. |
@FabrizioCaldarelli Yes, I agree about automatically feature that is way I change my proposal. |
@asamats I could have dependent models, but I could want to not update them when I save the parent model. |
@FabrizioCaldarelli What if we add a flag or new method for save parent and all depend models (if them added and updated)? |
@asamats It is not a AR responsability to do this work. |
@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 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.. |
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:
IMHO this logic must be implemented situationally as it is project-specific. |
I don't see any problem with implementation. |
I still think
|
@asamats why don't you create an extension to achieve it? |
@FabrizioCaldarelli because we have similar exts. I don't see any reason to create one other. |
@viktorprogger "it could be a good kick-up for startups, but a very big headache for big old projects." 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. |
I don't know how to name it, I just try to explain by example.
We have models like this:
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:
... forms and etc.
And when we get data by request from browser we just use load function:
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):
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']
The text was updated successfully, but these errors were encountered: