perf: Allow loading relations in other discussion endpoints #3191
Conversation
| @@ -98,9 +99,9 @@ protected function data(ServerRequestInterface $request, Document $document) | |||
| $this->includePosts($discussion, $request, $postRelationships); | |||
| } | |||
|
|
|||
| $discussion->load(array_filter($include, function ($relationship) { | |||
| $this->loadRelations(new Collection([$discussion]), array_filter($include, function ($relationship) { | |||
There was a problem hiding this comment.
I'm not sure that I like creating the collection here, it might be better to modify loadRelations to accept either a single model or a collection of models, as they both have the same methods, though I dislike that as well, I wish there was a common interface for this, there is a trait, but I don't think you can use traits for typing.
There was a problem hiding this comment.
Let's keep it like this for now; it's not perfect, but it's good enough.
re: typing, we can figure out the PHPStan implementation later.
There was a problem hiding this comment.
I'm not gonna lie, the complexity of this eager loading system has reached such a level that it is very difficult for me to fully understand what we're doing and why. I'm approving because we need to fix the performance issue, but I think this is a clear signal that we are limited by our current json-api-php usage, and yet another reason to completely redo the JSON:API layers of our stack for v2.
| @@ -98,9 +99,9 @@ protected function data(ServerRequestInterface $request, Document $document) | |||
| $this->includePosts($discussion, $request, $postRelationships); | |||
| } | |||
|
|
|||
| $discussion->load(array_filter($include, function ($relationship) { | |||
| $this->loadRelations(new Collection([$discussion]), array_filter($include, function ($relationship) { | |||
There was a problem hiding this comment.
Let's keep it like this for now; it's not perfect, but it's good enough.
re: typing, we can figure out the PHPStan implementation later.
askvortsov1
requested review from
clarkwinkelmann,
davwheat and
tankerkiller125
Related to #3116
Changes proposed in this pull request:
Missed the other two endpoints which also attempt to load the
tags.staterelationship. The impact is the same.Reviewers should focus on:
I really dislike the implementation in ShowDiscussionsController, it's becoming very bloated, and that's mostly due to how the posts of the discussion are loaded. I feel like it would be better to load those posts by directly getting the result from the posts endpoint, we should look into refactoring it in another major version.
Necessity
Confirmed
composer test).