-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
Get parent model from its child relation #328
Conversation
xyz1123581321
commented
Oct 6, 2021
•
edited by gitpod-io
bot
Loading
edited by gitpod-io
bot
If the parent relation is defined in its child relation, then assign its parent model to its child
If the parent relation is defined in its child relation, then assign its parent model to its child
Is it related to any issue? |
We talked about this small improvement on discord with Luke. |
Ok, just for prosperity I post here the issue from the OC forum you are trying to solve. Just for others who don't know like me what's going on here :)
|
Did you review it? |
@@ -657,7 +682,6 @@ protected function makeSearchWidget() | |||
protected function makeViewWidget() | |||
{ | |||
$widget = null; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to have whitespace added back in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change anything at 660...
Only 362 to 382.
/*
* Parent relationship
*/
$parentClass = get_class($this->model);
$allRelationDefinitions = $this->relationModel->getRelationDefinitions();
$parentClassName = "";
foreach ($allRelationDefinitions as $relations) {
foreach ($relations as $key => $value) {
if (strtolower($value[0]) == strtolower($parentClass)) {
$parentClassName = $key;
}
}
}
if ($parentClassName != "") {
$this->relationModel->{$parentClassName} = $this->model;
}
break; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit fragile to me, I hadn't realized the complexity of setting the inverse relationship. @bennothommo do you have any ideas for this?
Right now I'm concerned about how well it supports all of the relationship modes supported by the RelationController and also if the related model has multiple relationship definitions tying back to the original parent model, how would we support that / should we try to support that? I'm thinking our best bet for that issue is to explicitly detect any more than one possible inverse relationship and don't do anything if that's encountered. It's already a bit presumptuous to be setting the inverse relationship directly when it might have scopes or conditions limiting the current parent model from being set in the first place.
Or perhaps I'm just overthinking this all, @bennothommo any thoughts on the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bennothommo ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the extreme delay in responding to this one!
It is somewhat flimsy, but at the same time - conventionally speaking - we should be alright. I can't think of a better way right now, and even if I could, there's no telling whether it would work or not given our relation definition is completely different to Laravel's.
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days. |
This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months. |
This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months. |
@LukeTowers anything else required here? |
The way I do this personally is this in the related model constructor: public function __construct(array $attributes = [])
{
parent::__construct($attributes);
$this->bindEvent('model.form.filterFields', function ($formWidget, $fields, $context) {
if ($context === 'create') {
if ($model = $formWidget->model) {
$model->parent = $formWidget->getController()->relationObject->getParent();
}
}
}
} |
I'm concerned about knock-on effects from making this sort of change, is it possible to simplify the problem down to a small demo of the issue that can have test cases documenting the expectations and then verify the solution from there or is it too complicated with how it integrates with the relationcontroller functionality? Either way I'll need to spend some time on this to refresh my memory on what the original problem was as well as how this solved it and my original concerns. Is there still interest in getting this merged or is @mjauvin's proposed workaround decent enough for anyone else encountering this sort of bump in the road? |
This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months. |
I just use the following:
|
Let's close this for now, given there are available workarounds. If someone wishes to take charge of this and get this over the line, feel free to drop a line here and we'll re-open. |