fix alias relationships not firing its own query #612
Conversation
There was a problem hiding this comment.
I don't feel confident doing a good review on this one.
This shows the absurd complexity we've to put upon us because of the concept of the SelectFields and once more shows me this approach goes into the wrong direction somehow…
Leads me to believe SelectFields should be a standalone component and not in this library TBH 😄
Mutating models scopes, adding macros to builders .. this somehow looks dangerous to me 🤷♀️
When I think about I would implement a dedicated PHP server with this library (i.e. never restart, a daemon) I can foresee thus causing problems. Is there no other way to solve this aliasing / double-query problem?
Not really sure how to move forward with this one TBH.
Leads me to believe
SelectFieldsshould be a standalone component and not in this library TBH 😄
To clarify this, I believe the proper approach is to use a dataloader concept to solve these things in general (that's what I'm doing in my projects, I'm not using SelectFields)
| */ | ||
| public static function add(string $model, array $relationships): void | ||
| { | ||
| $model::addGlobalScope(new AddModelRelationshipScope($relationships)); |
There was a problem hiding this comment.
👀
Isn't that somehow dangerous (side-effects), permanently mutating the the models global scopes?
There was a problem hiding this comment.
I don't feel confident doing a good review on this one.
This shows the absurd complexity we've to put upon us because of the concept of the
SelectFieldsand once more shows me this approach goes into the wrong direction somehow…Leads me to believe
SelectFieldsshould be a standalone component and not in this library TBH 😄Mutating models scopes, adding macros to builders .. this somehow looks dangerous to me 🤷♀️
When I think about I would implement a dedicated PHP server with this library (i.e. never restart, a daemon) I can foresee thus causing problems. Is there no other way to solve this aliasing / double-query problem?
Not really sure how to move forward with this one TBH.
Leads me to believe
SelectFieldsshould be a standalone component and not in this library TBH 😄To clarify this, I believe the proper approach is to use a dataloader concept to solve these things in general (that's what I'm doing in my projects, I'm not using SelectFields)
I love the SelectFields and use it for 99% of my queries, makes it so easy to set up and think it is one of the main "selling points" of the libary
There was a problem hiding this comment.
👀
Isn't that somehow dangerous (side-effects), permanently mutating the the models global scopes?
yes it is definitely a side effect, but since SelectFields are made to be used with eloquent's ->with(). The only way to fix this is to generate the relationships.
The side effect is minimized by generating relationships with generated in front of all generated relationships, so I would say this makes it as safe as possible and only applied if alias are actually used
There was a problem hiding this comment.
yeah we might need to tore the relationship down if some one uses the same relationship again
| public function extend(Builder $builder): void | ||
| { | ||
| foreach ($this->relationships as [$graphqlAlias, $relationship]) { | ||
| $builder->macro($graphqlAlias, function (Builder $builder) use ($relationship) { |
| /** | ||
| * @var array<class-string<Model>,array<string,string>> | ||
| */ | ||
| private static $modelRelationshipsForAlias = []; |
There was a problem hiding this comment.
Wish this wouldn't be static.
I understand from the code it's cleaned up, still…
|
@crissi not sure if/how you want to move forward here. Besides all the complexity, for me the current no-go definitely are the side-effects of mutating the models behaviour (and also the macro thing); I don't think this is acceptable to be done from such a library. |
|
I see you point, but people would expect aliases to work since they are a feature in graphql. could we make it an opt in feature in the config? with a good description in documentation of what the risk of the feature could be. I want to tighten it up, after some of the cases you mention in the comments. |
|
Sorry to sound stubborn, but as long as the code mutates the models I don't want to accept this (config or not; and I think it's a bad idea to use a "config switch" for that). |
fair |
Summary
fixes #604
Type of change
Checklist